[U-Boot] [PATCH 1/2] spi: Add progress percentage and write speed to `sf update`

[U-Boot] [PATCH 1/2] spi: Add progress percentage and write speed to `sf update`

[Simon Glass]

From: James Miller <jamesmiller@chromium.org>

Output a progress update only at most 10 times per second, to avoid
saturating (and waiting on) the console. Make the summary line
to fit on a single line. Make sure that cursor sits at the end of
each update line instead of the beginning.

Sample output:

SF: Detected W25Q32 with page size 4 KiB, total 4 MiB
Update SPI
1331200 bytes written, 2863104 bytes skipped in 21.912s, speed 199728 B/s

time: 21.919 seconds, 21919 ticks
Skipping verify

Signed-off-by: Simon Glass <sjg@chromium.org>
Signed-off-by: James Miller <jamesmiller@chromium.org>
Signed-off-by: Taylor Hutt <thutt@chromium.org>
---
 common/cmd_sf.c |   41 +++++++++++++++++++++++++++++++++++++++--
 1 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/common/cmd_sf.c b/common/cmd_sf.c
index 5ac1d0c..e22a45e 100644
--- a/common/cmd_sf.c
+++ b/common/cmd_sf.c
@@ -67,6 +67,23 @@ static int sf_parse_len_arg(char *arg, ulong *len)
 	return 1;
 }
 
+/**
+ * This function takes a byte length and a delta unit of time to compute the
+ * approximate bytes per second
+ *
+ * @param len		amount of bytes currently processed
+ * @param start_ms	start time of processing in ms
+ * @return bytes per second if OK, 0 on error
+ */
+static const ulong bytes_per_second(unsigned int len, ulong start_ms)
+{
+	/* less accurate but avoids overflow */
+	if (len >= ((unsigned int) -1) / 1024)
+		return len / (max(get_timer(start_ms) / 1024, 1));
+	else
+		return 1024 * len / max(get_timer(start_ms), 1);
+}
+
 static int do_spi_flash_probe(int argc, char * const argv[])
 {
 	unsigned int bus = CONFIG_SF_DEFAULT_BUS;
@@ -167,11 +184,26 @@ static int spi_flash_update(struct spi_flash *flash, u32 offset,
 	const char *end = buf + len;
 	size_t todo;		/* number of bytes to do in this pass */
 	size_t skipped = 0;	/* statistics */
+	const ulong start_time = get_timer(0);
+	size_t scale = 1;
+	const char *start_buf = buf;
+	ulong delta;
 
+	if (end - buf >= 200)
+		scale = (end - buf) / 100;
 	cmp_buf = malloc(flash->sector_size);
 	if (cmp_buf) {
+		ulong last_update = get_timer(0);
+
 		for (; buf < end && !err_oper; buf += todo, offset += todo) {
 			todo = min(end - buf, flash->sector_size);
+			if (get_timer(last_update) > 100) {
+				printf("   \rUpdating, %zu%% %lu B/s",
+					100 - (end - buf) / scale,
+					bytes_per_second(buf - start_buf,
+							 start_time));
+				last_update = get_timer(0);
+			}
 			err_oper = spi_flash_update_block(flash, offset, todo,
 					buf, cmp_buf, &skipped);
 		}
@@ -179,12 +211,17 @@ static int spi_flash_update(struct spi_flash *flash, u32 offset,
 		err_oper = "malloc";
 	}
 	free(cmp_buf);
+	putc('\r');
 	if (err_oper) {
 		printf("SPI flash failed in %s step\n", err_oper);
 		return 1;
 	}
-	printf("%zu bytes written, %zu bytes skipped\n", len - skipped,
-	       skipped);
+
+	delta = get_timer(start_time);
+	printf("%zu bytes written, %zu bytes skipped", len - skipped,
+		skipped);
+	printf(" in %ld.%lds, speed %ld B/s\n",
+		delta / 1000, delta % 1000, bytes_per_second(len, start_time));
 
 	return 0;
 }
-- 
1.7.7.3

[U-Boot] [PATCH 2/2] spi: Add SPI flash test

[Simon Glass]

It is useful to have a basic SPI flash test, which tests that the SPI chip,
the SPI bus and the driver are behaving.

This test erases part of the flash, writes data and reads it back as a
sanity check that all is well.

Use CONFIG_SF_TEST to enable it.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 README          |    5 ++
 common/cmd_sf.c |  160 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 165 insertions(+), 0 deletions(-)

diff --git a/README b/README
index 5793b0a..8f601ae 100644
--- a/README
+++ b/README
@@ -2309,6 +2309,11 @@ The following options need to be configured:
 		CONFIG_SF_DEFAULT_MODE 		(see include/spi.h)
 		CONFIG_SF_DEFAULT_SPEED		in Hz
 
+		CONFIG_CMD_SF_TEST
+
+		Define this option to include a destructive SPI flash
+		test ('sf test').
+
 - SystemACE Support:
 		CONFIG_SYSTEMACE
 
diff --git a/common/cmd_sf.c b/common/cmd_sf.c
index e22a45e..2462c25 100644
--- a/common/cmd_sf.c
+++ b/common/cmd_sf.c
@@ -5,6 +5,7 @@
  * Licensed under the GPL-2 or later.
  */
 
+#include <div64.h>
 #include <common.h>
 #include <malloc.h>
 #include <spi_flash.h>
@@ -312,6 +313,153 @@ static int do_spi_flash_erase(int argc, char * const argv[])
 	return 0;
 }
 
+#ifdef CONFIG_CMD_SF_TEST
+enum {
+	STAGE_ERASE,
+	STAGE_CHECK,
+	STAGE_WRITE,
+	STAGE_READ,
+
+	STAGE_COUNT,
+};
+
+static char *stage_name[STAGE_COUNT] = {
+	"erase",
+	"check",
+	"write",
+	"read",
+};
+
+struct test_info {
+	int stage;
+	int bytes;
+	unsigned base_ms;
+	unsigned time_ms[STAGE_COUNT];
+};
+
+static void show_time(struct test_info *test, int stage)
+{
+	uint64_t speed;	/* KiB/s */
+	int bps;	/* Bits per second */
+
+	speed = (long long)test->bytes * 1000;
+	do_div(speed, test->time_ms[stage] * 1024);
+	bps = speed * 8;
+
+	printf("%d %s: %d: %d KiB/s %d.%03d Mbps\n", stage, stage_name[stage],
+	       test->time_ms[stage], (int)speed, bps / 1000, bps % 1000);
+}
+
+static void spi_test_next_stage(struct test_info *test)
+{
+	test->time_ms[test->stage] = get_timer(test->base_ms);
+	show_time(test, test->stage);
+	test->base_ms = get_timer(0);
+	test->stage++;
+}
+
+/**
+ * Run a test on the SPI flash
+ *
+ * @param flash		SPI flash to use
+ * @param buf		Source buffer for data to write
+ * @param len		Size of data to read/write
+ * @param offset	Offset within flash to check
+ * @param vbuf		Verification buffer
+ * @return 0 if ok, -1 on error
+ */
+static int spi_flash_test(struct spi_flash *flash, char *buf, ulong len,
+			   ulong offset, char *vbuf)
+{
+	struct test_info test;
+	int i;
+
+	printf("SPI flash test:\n");
+	memset(&test, '\0', sizeof(test));
+	test.base_ms = get_timer(0);
+	test.bytes = len;
+	if (spi_flash_erase(flash, offset, len)) {
+		printf("Erase failed\n");
+		return -1;
+	}
+	spi_test_next_stage(&test);
+
+	if (spi_flash_read(flash, offset, len, vbuf)) {
+		printf("Check read failed\n");
+		return -1;
+	}
+	for (i = 0; i < len; i++) {
+		if (vbuf[i] != 0xff) {
+			printf("Check failed at %d\n", i);
+			print_buffer(i, vbuf + i, 1, min(len - i, 0x40), 0);
+			return -1;
+		}
+	}
+	spi_test_next_stage(&test);
+
+	if (spi_flash_write(flash, offset, len, buf)) {
+		printf("Write failed\n");
+		return -1;
+	}
+	memset(vbuf, '\0', len);
+	spi_test_next_stage(&test);
+
+	if (spi_flash_read(flash, offset, len, vbuf)) {
+		printf("Read failed\n");
+		return -1;
+	}
+	spi_test_next_stage(&test);
+
+	for (i = 0; i < len; i++) {
+		if (buf[i] != vbuf[i]) {
+			printf("Verify failed at %d, good data:\n", i);
+			print_buffer(i, buf + i, 1, min(len - i, 0x40), 0);
+			printf("Bad data:\n");
+			print_buffer(i, vbuf + i, 1, min(len - i, 0x40), 0);
+			return -1;
+		}
+	}
+	printf("Test passed\n");
+	for (i = 0; i < STAGE_COUNT; i++)
+		show_time(&test, i);
+
+	return 0;
+}
+
+static int do_spi_flash_test(void)
+{
+	/* TODO(sjg at chromium.org): Support cmdline parameters for these */
+	unsigned long offset = 0x8000;
+	unsigned long len = 0x10000;
+	char *buf = (char *)CONFIG_SYS_TEXT_BASE;
+	char *vbuf;
+	int ret;
+
+	vbuf = malloc(len);
+	if (!vbuf) {
+		printf("Cannot allocate memory\n");
+		return 1;
+	}
+	buf = malloc(len);
+	if (!buf) {
+		free(vbuf);
+		printf("Cannot allocate memory\n");
+		return 1;
+	}
+
+	memcpy(buf, (char *)CONFIG_SYS_TEXT_BASE, len);
+	ret = spi_flash_test(flash, buf, len, offset, vbuf);
+	free(vbuf);
+	free(buf);
+	if (ret) {
+		printf("Test failed\n");
+		return 1;
+	}
+
+	return 0;
+}
+#endif /* CONFIG_CMD_SF_TEST */
+
 static int do_spi_flash(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
 	const char *cmd;
@@ -341,6 +489,10 @@ static int do_spi_flash(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[
 		ret = do_spi_flash_read_write(argc, argv);
 	else if (strcmp(cmd, "erase") == 0)
 		ret = do_spi_flash_erase(argc, argv);
+#ifdef CONFIG_CMD_SF_TEST
+	else if (!strcmp(cmd, "test"))
+		ret = do_spi_flash_test();
+#endif
 	else
 		ret = -1;
 
@@ -352,6 +504,13 @@ usage:
 	return CMD_RET_USAGE;
 }
 
+#ifdef CONFIG_CMD_SF_TEST
+#define SF_TEST_HELP "\nsf test				" \
+		"- 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",
@@ -365,4 +524,5 @@ U_BOOT_CMD(
 	"				  `+len' round up `len' to block size\n"
 	"sf update addr offset len	- erase and write `len' bytes from memory\n"
 	"				  at `addr' to flash@`offset'"
+	SF_TEST_HELP
 );
-- 
1.7.7.3

[U-Boot] [PATCH 2/2] spi: Add SPI flash test

[Tom Rini]

On Fri, Sep 28, 2012 at 05:28:01PM -0700, Simon Glass wrote:

> It is useful to have a basic SPI flash test, which tests that the SPI chip,
> the SPI bus and the driver are behaving.
> 
> This test erases part of the flash, writes data and reads it back as a
> sanity check that all is well.
> 
> Use CONFIG_SF_TEST to enable it.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>  README          |    5 ++
>  common/cmd_sf.c |  160 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 165 insertions(+), 0 deletions(-)
> 
> diff --git a/README b/README
> index 5793b0a..8f601ae 100644
> --- a/README
> +++ b/README
> @@ -2309,6 +2309,11 @@ The following options need to be configured:
>  		CONFIG_SF_DEFAULT_MODE 		(see include/spi.h)
>  		CONFIG_SF_DEFAULT_SPEED		in Hz
>  
> +		CONFIG_CMD_SF_TEST
> +
> +		Define this option to include a destructive SPI flash
> +		test ('sf test').
> +

Lets make this note as well that it is of course a destructive test.

[snip]
> +static int do_spi_flash_test(void)
> +{
> +	/* TODO(sjg at chromium.org): Support cmdline parameters for these */

Lets just add that now? :)  Thanks.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20121001/ada1408f/attachment.pgp>

[U-Boot] [PATCH 2/2] spi: Add SPI flash test

[Simon Glass]

Hi Tom,

On Mon, Oct 1, 2012 at 10:32 AM, Tom Rini <trini@ti.com> wrote:
> On Fri, Sep 28, 2012 at 05:28:01PM -0700, Simon Glass wrote:
>
>> It is useful to have a basic SPI flash test, which tests that the SPI chip,
>> the SPI bus and the driver are behaving.
>>
>> This test erases part of the flash, writes data and reads it back as a
>> sanity check that all is well.
>>
>> Use CONFIG_SF_TEST to enable it.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>>  README          |    5 ++
>>  common/cmd_sf.c |  160 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 165 insertions(+), 0 deletions(-)
>>
>> diff --git a/README b/README
>> index 5793b0a..8f601ae 100644
>> --- a/README
>> +++ b/README
>> @@ -2309,6 +2309,11 @@ The following options need to be configured:
>>               CONFIG_SF_DEFAULT_MODE          (see include/spi.h)
>>               CONFIG_SF_DEFAULT_SPEED         in Hz
>>
>> +             CONFIG_CMD_SF_TEST
>> +
>> +             Define this option to include a destructive SPI flash
>> +             test ('sf test').
>> +
>
> Lets make this note as well that it is of course a destructive test.

Do you mean change the comment?

>
> [snip]
>> +static int do_spi_flash_test(void)
>> +{
>> +     /* TODO(sjg at chromium.org): Support cmdline parameters for these */
>
> Lets just add that now? :)  Thanks.

Fair enough, will do.

Regards,
Simon

>
> --
> Tom

[U-Boot] [PATCH 2/2] spi: Add SPI flash test

[Tom Rini]

On Mon, Oct 08, 2012 at 04:00:08PM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Mon, Oct 1, 2012 at 10:32 AM, Tom Rini <trini@ti.com> wrote:
> > On Fri, Sep 28, 2012 at 05:28:01PM -0700, Simon Glass wrote:
> >
> >> It is useful to have a basic SPI flash test, which tests that the SPI chip,
> >> the SPI bus and the driver are behaving.
> >>
> >> This test erases part of the flash, writes data and reads it back as a
> >> sanity check that all is well.
> >>
> >> Use CONFIG_SF_TEST to enable it.
> >>
> >> Signed-off-by: Simon Glass <sjg@chromium.org>
> >> ---
> >>  README          |    5 ++
> >>  common/cmd_sf.c |  160 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  2 files changed, 165 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/README b/README
> >> index 5793b0a..8f601ae 100644
> >> --- a/README
> >> +++ b/README
> >> @@ -2309,6 +2309,11 @@ The following options need to be configured:
> >>               CONFIG_SF_DEFAULT_MODE          (see include/spi.h)
> >>               CONFIG_SF_DEFAULT_SPEED         in Hz
> >>
> >> +             CONFIG_CMD_SF_TEST
> >> +
> >> +             Define this option to include a destructive SPI flash
> >> +             test ('sf test').
> >> +
> >
> > Lets make this note as well that it is of course a destructive test.
> 
> Do you mean change the comment?

Um.  I had reading comprehension issues that morning.  I can see right
now that it says it's destructive.

> > [snip]
> >> +static int do_spi_flash_test(void)
> >> +{
> >> +     /* TODO(sjg at chromium.org): Support cmdline parameters for these */
> >
> > Lets just add that now? :)  Thanks.
> 
> Fair enough, will do.

Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20121008/ffb4608d/attachment.pgp>

[U-Boot] [PATCH 2/2] spi: Add SPI flash test

[Wolfgang Denk]

Dear Simon Glass,

In message <1348878482-1730-2-git-send-email-sjg@chromium.org> you wrote:
> It is useful to have a basic SPI flash test, which tests that the SPI chip,
> the SPI bus and the driver are behaving.
> 
> This test erases part of the flash, writes data and reads it back as a
> sanity check that all is well.
> 
> Use CONFIG_SF_TEST to enable it.

Adding dead code ?

Please don't.

Best regards,

Wolfgang Denk

--
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
No, I'm not going to explain it. If you  can't  figure  it  out,  you
didn't want to know anyway... :-)
                   - Larry Wall in <1991Aug7.180856.2854@netlabs.com>

[U-Boot] [U-Boot, 1/2] spi: Add progress percentage and write speed to `sf update`

[Tom Rini]

On Fri, Sep 28, 2012 at 02:28:00PM -0000, Simon Glass wrote:

> From: James Miller <jamesmiller@chromium.org>
> 
> Output a progress update only at most 10 times per second, to avoid
> saturating (and waiting on) the console. Make the summary line
> to fit on a single line. Make sure that cursor sits at the end of
> each update line instead of the beginning.
> 
> Sample output:
> 
> SF: Detected W25Q32 with page size 4 KiB, total 4 MiB
> Update SPI
> 1331200 bytes written, 2863104 bytes skipped in 21.912s, speed 199728 B/s
> 
> time: 21.919 seconds, 21919 ticks
> Skipping verify
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: James Miller <jamesmiller@chromium.org>
> Signed-off-by: Taylor Hutt <thutt@chromium.org>

With ELDK 4.2:
cmd_sf.c:80: warning: type qualifiers ignored on function return type

Unless I'm missing something, this is the right fix:
-static const ulong bytes_per_second(unsigned int len, ulong start_ms)
+static ulong bytes_per_second(unsigned int len, ulong start_ms)

Because no, we aren't returning a const.  If that sounds right I'll fix
inline and add my Signed-off-by.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20121219/cfdfea3a/attachment.pgp>

[U-Boot] [U-Boot, 1/2] spi: Add progress percentage and write speed to `sf update`

[Simon Glass]

Hi Tom,

On Wed, Dec 19, 2012 at 12:43 PM, Tom Rini <trini@ti.com> wrote:
> On Fri, Sep 28, 2012 at 02:28:00PM -0000, Simon Glass wrote:
>
>> From: James Miller <jamesmiller@chromium.org>
>>
>> Output a progress update only at most 10 times per second, to avoid
>> saturating (and waiting on) the console. Make the summary line
>> to fit on a single line. Make sure that cursor sits at the end of
>> each update line instead of the beginning.
>>
>> Sample output:
>>
>> SF: Detected W25Q32 with page size 4 KiB, total 4 MiB
>> Update SPI
>> 1331200 bytes written, 2863104 bytes skipped in 21.912s, speed 199728 B/s
>>
>> time: 21.919 seconds, 21919 ticks
>> Skipping verify
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> Signed-off-by: James Miller <jamesmiller@chromium.org>
>> Signed-off-by: Taylor Hutt <thutt@chromium.org>
>
> With ELDK 4.2:
> cmd_sf.c:80: warning: type qualifiers ignored on function return type

Interesting...

>
> Unless I'm missing something, this is the right fix:
> -static const ulong bytes_per_second(unsigned int len, ulong start_ms)
> +static ulong bytes_per_second(unsigned int len, ulong start_ms)
>
> Because no, we aren't returning a const.  If that sounds right I'll fix
> inline and add my Signed-off-by.

Yes that looks right, thanks for doing that. Did Mike get in touch re these?

Regards,
Simon

>
> --
> Tom

[U-Boot] [U-Boot, 1/2] spi: Add progress percentage and write speed to `sf update`

[Tom Rini]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 12/19/12 15:46, Simon Glass wrote:
> Hi Tom,
> 
> On Wed, Dec 19, 2012 at 12:43 PM, Tom Rini <trini@ti.com> wrote:
>> On Fri, Sep 28, 2012 at 02:28:00PM -0000, Simon Glass wrote:
>> 
>>> From: James Miller <jamesmiller@chromium.org>
>>> 
>>> Output a progress update only at most 10 times per second, to 
>>> avoid saturating (and waiting on) the console. Make the
>>> summary line to fit on a single line. Make sure that cursor
>>> sits at the end of each update line instead of the beginning.
>>> 
>>> Sample output:
>>> 
>>> SF: Detected W25Q32 with page size 4 KiB, total 4 MiB Update 
>>> SPI 1331200 bytes written, 2863104 bytes skipped in 21.912s, 
>>> speed 199728 B/s
>>> 
>>> time: 21.919 seconds, 21919 ticks Skipping verify
>>> 
>>> Signed-off-by: Simon Glass <sjg@chromium.org> Signed-off-by: 
>>> James Miller <jamesmiller@chromium.org> Signed-off-by: Taylor 
>>> Hutt <thutt@chromium.org>
>> 
>> With ELDK 4.2: cmd_sf.c:80: warning: type qualifiers ignored on 
>> function return type
> 
> Interesting...
> 
>> 
>> Unless I'm missing something, this is the right fix: -static 
>> const ulong bytes_per_second(unsigned int len, ulong start_ms) 
>> +static ulong bytes_per_second(unsigned int len, ulong start_ms)
>> 
>> Because no, we aren't returning a const.  If that sounds right 
>> I'll fix inline and add my Signed-off-by.
> 
> Yes that looks right, thanks for doing that. Did Mike get in touch 
> re these?

No, but I've read them again myself and they seem sane enough to grab
as Mike hasn't been active recently.  And I'll push this shortly then,
thanks!

- -- 
Tom
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://www.enigmail.net/

iQIcBAEBAgAGBQJQ0irGAAoJENk4IS6UOR1WVJIQAJfcEUS0JA6+cp5D+aEt+mbC
fnXcNB9iqqTJ+7s4FBrC7Q48MZooiynjbwo9M2/KrtjCtoKwM3pqHcvCZpL1R1l0
nJaGjgDgCU+DvXiWVRSGKYHan+O/EOYbzFxqDvS7y9A3+wJwBf3yvvom5/CavV61
pEzjSxaUdA0Ew4NZ7b3cVXqcDN00dzsuH1BgtRdNsaGauVJNtfi+83fHLZJfl8cY
24hC4iGqQu8hj13pAO4TMmsCYGVeXU+sSoTWzdYt6aYVu9nQUoeOplyUy+9e53Vr
v+1cgqiArqkIPLa0ypLGzR2f1QVzZqndXkRCmGNq0iVSowpfCPrDQO2OowzqmBEl
b9WUzmo3nkEn1pfY8EGYYtYXa8yyjie0qSzgbIiepBJFequPyzq/D2lY2a2buP2g
nUHMBS/fp04KunuY4PPrJoqXhUJFDSdsKhZBQS+hi3hzNpaT8u9D2QyjgiT/3mya
B92T4D9VvuLutpwqfGDiKBc7iwVrARBAxNUU9O82pCrV9l1+P669czqJWfARIdvy
Zpb1DGwudW1L8VEo0gR+wjp/bU2IxSSKBvRGrPENhklM4+121Jf2Wxbc7qq0XRik
8aRKL5cN3pWN2NoIuxEg0F3FRTrw24vqFv6J87jwCOZ7OCRsP47n2nzQXi1766UY
jRvezzCuDRQetEc1Rx1g
=WeUR
-----END PGP SIGNATURE-----

[U-Boot] [U-Boot, 1/2] spi: Add progress percentage and write speed to `sf update`

[Tom Rini]

On Wed, Dec 19, 2012 at 03:59:50PM -0500, Tom Rini wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 12/19/12 15:46, Simon Glass wrote:
> > Hi Tom,
> > 
> > On Wed, Dec 19, 2012 at 12:43 PM, Tom Rini <trini@ti.com> wrote:
> >> On Fri, Sep 28, 2012 at 02:28:00PM -0000, Simon Glass wrote:
> >> 
> >>> From: James Miller <jamesmiller@chromium.org>
> >>> 
> >>> Output a progress update only at most 10 times per second, to 
> >>> avoid saturating (and waiting on) the console. Make the
> >>> summary line to fit on a single line. Make sure that cursor
> >>> sits at the end of each update line instead of the beginning.
> >>> 
> >>> Sample output:
> >>> 
> >>> SF: Detected W25Q32 with page size 4 KiB, total 4 MiB Update 
> >>> SPI 1331200 bytes written, 2863104 bytes skipped in 21.912s, 
> >>> speed 199728 B/s
> >>> 
> >>> time: 21.919 seconds, 21919 ticks Skipping verify
> >>> 
> >>> Signed-off-by: Simon Glass <sjg@chromium.org> Signed-off-by: 
> >>> James Miller <jamesmiller@chromium.org> Signed-off-by: Taylor 
> >>> Hutt <thutt@chromium.org>
> >> 
> >> With ELDK 4.2: cmd_sf.c:80: warning: type qualifiers ignored on 
> >> function return type
> > 
> > Interesting...
> > 
> >> 
> >> Unless I'm missing something, this is the right fix: -static 
> >> const ulong bytes_per_second(unsigned int len, ulong start_ms) 
> >> +static ulong bytes_per_second(unsigned int len, ulong start_ms)
> >> 
> >> Because no, we aren't returning a const.  If that sounds right 
> >> I'll fix inline and add my Signed-off-by.
> > 
> > Yes that looks right, thanks for doing that. Did Mike get in touch 
> > re these?
> 
> No, but I've read them again myself and they seem sane enough to grab
> as Mike hasn't been active recently.  And I'll push this shortly then,
> thanks!

With this change, applied to u-boot/master.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20121219/e8838804/attachment.pgp>

[U-Boot] [U-Boot, 1/2] spi: Add progress percentage and write speed to `sf update`

[Wolfgang Denk]

Dear Tom Rini,

In message <20121219225945.GF14589@bill-the-cat> you wrote:
> 
...
> With this change, applied to u-boot/master.

Argh.... :-(

Can we please undo this somehow?  This does not fit at all
conceptually.  U-Boot is supposed to use the good ols UNIX philosophy
of being terse by default, and special casing one specific storage
device makes no sense at all to me.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
I am pleased to see that we have differences.  May we together become
greater than the sum of both of us.
	-- Surak of Vulcan, "The Savage Curtain", stardate 5906.4

[U-Boot] [U-Boot, 1/2] spi: Add progress percentage and write speed to `sf update`

[Tom Rini]

On Thu, Dec 20, 2012 at 12:14:00AM +0100, Wolfgang Denk wrote:
> Dear Tom Rini,
> 
> In message <20121219225945.GF14589@bill-the-cat> you wrote:
> > 
> ...
> > With this change, applied to u-boot/master.
> 
> Argh.... :-(
> 
> Can we please undo this somehow?  This does not fit at all
> conceptually.  U-Boot is supposed to use the good ols UNIX philosophy
> of being terse by default, and special casing one specific storage
> device makes no sense at all to me.

We need to fix some of the underlying problems so that we're consistent
here.  Sometimes we have output (network #), sometimes we don't.
Sometimes we have a speed (network, filesystem load), sometimes we
don't.  I'd be quite happy to have a uniform output and a uniform ON/OFF
switch.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20121219/c1e2e705/attachment.pgp>

[U-Boot] [U-Boot, 1/2] spi: Add progress percentage and write speed to `sf update`

[Simon Glass]

Hi Tom,

On Wed, Dec 19, 2012 at 5:03 PM, Tom Rini <trini@ti.com> wrote:
> On Thu, Dec 20, 2012 at 12:14:00AM +0100, Wolfgang Denk wrote:
>> Dear Tom Rini,
>>
>> In message <20121219225945.GF14589@bill-the-cat> you wrote:
>> >
>> ...
>> > With this change, applied to u-boot/master.
>>
>> Argh.... :-(
>>
>> Can we please undo this somehow?  This does not fit at all
>> conceptually.  U-Boot is supposed to use the good ols UNIX philosophy
>> of being terse by default, and special casing one specific storage
>> device makes no sense at all to me.
>
> We need to fix some of the underlying problems so that we're consistent
> here.  Sometimes we have output (network #), sometimes we don't.
> Sometimes we have a speed (network, filesystem load), sometimes we
> don't.  I'd be quite happy to have a uniform output and a uniform ON/OFF
> switch.

I'm happy to do something like this. Obviously we want a config, but
do we also want an env variable to control it? Could be useful.

And at the risk of killing it with feature creep, perhaps we could
have two levels of verbosity: progress (which repeatedly updates on
the same line) and notice (which does not). That might take care of
Jagannadha's use case also.

Regards,
Simon

>
> --
> Tom
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>

[U-Boot] [U-Boot, 1/2] spi: Add progress percentage and write speed to `sf update`

[Tom Rini]

On Wed, Dec 19, 2012 at 05:18:55PM -0800, Simon Glass wrote:
> Hi Tom,
> 
> On Wed, Dec 19, 2012 at 5:03 PM, Tom Rini <trini@ti.com> wrote:
> > On Thu, Dec 20, 2012 at 12:14:00AM +0100, Wolfgang Denk wrote:
> >> Dear Tom Rini,
> >>
> >> In message <20121219225945.GF14589@bill-the-cat> you wrote:
> >> >
> >> ...
> >> > With this change, applied to u-boot/master.
> >>
> >> Argh.... :-(
> >>
> >> Can we please undo this somehow?  This does not fit at all
> >> conceptually.  U-Boot is supposed to use the good ols UNIX philosophy
> >> of being terse by default, and special casing one specific storage
> >> device makes no sense at all to me.
> >
> > We need to fix some of the underlying problems so that we're consistent
> > here.  Sometimes we have output (network #), sometimes we don't.
> > Sometimes we have a speed (network, filesystem load), sometimes we
> > don't.  I'd be quite happy to have a uniform output and a uniform ON/OFF
> > switch.
> 
> I'm happy to do something like this. Obviously we want a config, but
> do we also want an env variable to control it? Could be useful.

The biggest blocker I see is that we should start the series by
re-orging things, if we can, so that we don't have this code in N
places.

> And at the risk of killing it with feature creep, perhaps we could
> have two levels of verbosity: progress (which repeatedly updates on
> the same line) and notice (which does not). That might take care of
> Jagannadha's use case also.

If we can do it such that it's (a) clean looking and (b) build-time
configurable too, I don't see why we can't give it a look at least.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20121220/131a8724/attachment.pgp>

[U-Boot] [U-Boot, 1/2] spi: Add progress percentage and write speed to `sf update`

[Jagan Teki]

Hi Simon,

On Thu, Dec 20, 2012 at 6:48 AM, Simon Glass <sjg@chromium.org> wrote:
> Hi Tom,
>
> On Wed, Dec 19, 2012 at 5:03 PM, Tom Rini <trini@ti.com> wrote:
>> On Thu, Dec 20, 2012 at 12:14:00AM +0100, Wolfgang Denk wrote:
>>> Dear Tom Rini,
>>>
>>> In message <20121219225945.GF14589@bill-the-cat> you wrote:
>>> >
>>> ...
>>> > With this change, applied to u-boot/master.
>>>
>>> Argh.... :-(
>>>
>>> Can we please undo this somehow?  This does not fit at all
>>> conceptually.  U-Boot is supposed to use the good ols UNIX philosophy
>>> of being terse by default, and special casing one specific storage
>>> device makes no sense at all to me.
>>
>> We need to fix some of the underlying problems so that we're consistent
>> here.  Sometimes we have output (network #), sometimes we don't.
>> Sometimes we have a speed (network, filesystem load), sometimes we
>> don't.  I'd be quite happy to have a uniform output and a uniform ON/OFF
>> switch.
>
> I'm happy to do something like this. Obviously we want a config, but
> do we also want an env variable to control it? Could be useful.
>
> And at the risk of killing it with feature creep, perhaps we could
> have two levels of verbosity: progress (which repeatedly updates on
> the same line) and notice (which does not). That might take care of
> Jagannadha's use case also.
>

Any plan to add config for verbose messages on cmd_sf.c?
seems like you plan for something, because I have some couple of patches
which has verbose messages for sf read/erase/write commands.
http://article.gmane.org/gmane.comp.boot-loaders.u-boot/149753

Thanks,
Jagan.

> Regards,
> Simon
>
>>
>> --
>> Tom
>>
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot at lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot
>>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [U-Boot, 1/2] spi: Add progress percentage and write speed to `sf update`

[Simon Glass]

Hi Jagan,

On Fri, Dec 21, 2012 at 12:46 AM, Jagan Teki <jagannadh.teki@gmail.com>wrote:

> Hi Simon,
>
> On Thu, Dec 20, 2012 at 6:48 AM, Simon Glass <sjg@chromium.org> wrote:
> > Hi Tom,
> >
> > On Wed, Dec 19, 2012 at 5:03 PM, Tom Rini <trini@ti.com> wrote:
> >> On Thu, Dec 20, 2012 at 12:14:00AM +0100, Wolfgang Denk wrote:
> >>> Dear Tom Rini,
> >>>
> >>> In message <20121219225945.GF14589@bill-the-cat> you wrote:
> >>> >
> >>> ...
> >>> > With this change, applied to u-boot/master.
> >>>
> >>> Argh.... :-(
> >>>
> >>> Can we please undo this somehow?  This does not fit at all
> >>> conceptually.  U-Boot is supposed to use the good ols UNIX philosophy
> >>> of being terse by default, and special casing one specific storage
> >>> device makes no sense at all to me.
> >>
> >> We need to fix some of the underlying problems so that we're consistent
> >> here.  Sometimes we have output (network #), sometimes we don't.
> >> Sometimes we have a speed (network, filesystem load), sometimes we
> >> don't.  I'd be quite happy to have a uniform output and a uniform ON/OFF
> >> switch.
> >
> > I'm happy to do something like this. Obviously we want a config, but
> > do we also want an env variable to control it? Could be useful.
> >
> > And at the risk of killing it with feature creep, perhaps we could
> > have two levels of verbosity: progress (which repeatedly updates on
> > the same line) and notice (which does not). That might take care of
> > Jagannadha's use case also.
> >
>
> Any plan to add config for verbose messages on cmd_sf.c?
> seems like you plan for something, because I have some couple of patches
> which has verbose messages for sf read/erase/write commands.
> http://article.gmane.org/gmane.comp.boot-loaders.u-boot/149753


Yes there seems to be a plan. Perhaps I will sketch out a few ideas so that
people can comment:

Add two environment variables:

verbose=0|1    - if this is 0, then commands complete silently as now. If
>=1 then messages like the ones you propose ('flash successfully erased')
appear
progress=0|1    - if this is 0, then commands show no progress when
working. If >=1 then some commands will show progress as they work (all on
a single line like 'sf update')

We also need a CONFIG for each to enable it, like perhaps
CONFIG_SYS_VERBOSE and CONFIG_SYS_PROGRESS.

Then I suppose we need a few functions like:

struct progress_info {
   uint count;   /* Number of things we are going to count through */
   uint upto;    /* Current thing we are up to */
   int flags;
   /* flags like whether to convert to KB/MB/etc., and whether to show
bytes per second */
   /* may need to record column position of last message so we can clear it
at the end */
};

void progress_setup(struct progress_info *progress, uint count, int flags);
void progress_update(struct progress_info *progress, uint upto);
void progress_done(struct progress_info *progress)
void verbose_printf()

which compile to nothing if the CONFIG options are not enabled. Will need
to look at each current instance of this sort of code though and make sure
that this covers everything that is needed.

I haven't done anything on this and won't get to it for several weeks,
which is why I am posting ideas here for comment.

Regards,
Simon


>
>
> Thanks,
> Jagan.
>
> > Regards,
> > Simon
> >
> >>
> >> --
> >> Tom
> >>
> >> _______________________________________________
> >> U-Boot mailing list
> >> U-Boot at lists.denx.de
> >> http://lists.denx.de/mailman/listinfo/u-boot
> >>
> > _______________________________________________
> > U-Boot mailing list
> > U-Boot at lists.denx.de
> > http://lists.denx.de/mailman/listinfo/u-boot
>

[U-Boot] [U-Boot, 1/2] spi: Add progress percentage and write speed to `sf update`

[Wolfgang Denk]

Dear Simon,

In message <CAPnjgZ35OaUPvMcc7Z_GHL4awWkxVZPHoOpEQjDLe0haP4e7cw@mail.gmail.com> you wrote:
>
> Yes there seems to be a plan. Perhaps I will sketch out a few ideas so that
> people can comment:

Thanks!

> Add two environment variables:
> 
> verbose=0|1    - if this is 0, then commands complete silently as now. If
> >=1 then messages like the ones you propose ('flash successfully erased')
> appear

If verbose is not set in the environment, the effect should be the
same as for "verbose=0".

It may even make sense to support other numeric values, too, so you
can even control the level of verbocity.

> progress=0|1    - if this is 0, then commands show no progress when
> working. If >=1 then some commands will show progress as they work (all on
> a single line like 'sf update')

Ditto for default if not set.

We should very much try to avoid the use of control sequences
including '\b' or '\r' characters.  These are a nightmare when
analyzing log files, not to mention the pain they often cause in
automatic regression test scripts.

If this makes single line output impossible (like when the total file
size is not known in advance so we can adjust the scaling), then I'd
rather see multiple lines (as we have now with the tftp / nfs
commands).

> We also need a CONFIG for each to enable it, like perhaps
> CONFIG_SYS_VERBOSE and CONFIG_SYS_PROGRESS.

This should be no _SYS, as it should be user-selectable.

>    /* may need to record column position of last message so we can clear it
> at the end */

Clear? Just print a '\n' and continue on a new line, please.

Thanks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
EMACS belongs in <sys/errno.h>: Editor too big!

[U-Boot] [PATCH 1/2] spi: Add progress percentage and write speed to `sf update`

[Wolfgang Denk]

Dear Simon Glass,

In message <1348878482-1730-1-git-send-email-sjg@chromium.org> you wrote:
> From: James Miller <jamesmiller@chromium.org>
> 
> Output a progress update only at most 10 times per second, to avoid
> saturating (and waiting on) the console. Make the summary line
> to fit on a single line. Make sure that cursor sits at the end of
> each update line instead of the beginning.
> 
> Sample output:
> 
> SF: Detected W25Q32 with page size 4 KiB, total 4 MiB
> Update SPI
> 1331200 bytes written, 2863104 bytes skipped in 21.912s, speed 199728 B/s

I dislike making commands more verbose then needed, or helpful.  Of
course the latter may be considered a matter of taste, but first of
all you also add code size here for questionable benefit.

I object against this patch:

1) I cannot see what is so special in the "sf" command that it needs
   such handling, while commands accessing NOR or NAND flash or
   SDCard or any other storage devices don't.

   If there is an agreement that this feature should be added, then it
   should be done in a general way that can be used everywhere.

   [Note that I doubt that "if".]

2) The code size hurts (at least in some cases). Such code should be
   optional. Please make it configurable.


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
A witty saying proves nothing, but saying  something  pointless  gets
people's attention.

[U-Boot] [PATCH 1/2] spi: Add progress percentage and write speed to `sf update`

[Simon Glass]

Hi Wolfgang,

On Wed, Dec 19, 2012 at 3:10 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Simon Glass,
>
> In message <1348878482-1730-1-git-send-email-sjg@chromium.org> you wrote:
>> From: James Miller <jamesmiller@chromium.org>
>>
>> Output a progress update only at most 10 times per second, to avoid
>> saturating (and waiting on) the console. Make the summary line
>> to fit on a single line. Make sure that cursor sits at the end of
>> each update line instead of the beginning.
>>
>> Sample output:
>>
>> SF: Detected W25Q32 with page size 4 KiB, total 4 MiB
>> Update SPI
>> 1331200 bytes written, 2863104 bytes skipped in 21.912s, speed 199728 B/s
>
> I dislike making commands more verbose then needed, or helpful.  Of
> course the latter may be considered a matter of taste, but first of
> all you also add code size here for questionable benefit.
>
> I object against this patch:
>
> 1) I cannot see what is so special in the "sf" command that it needs
>    such handling, while commands accessing NOR or NAND flash or
>    SDCard or any other storage devices don't.
>
>    If there is an agreement that this feature should be added, then it
>    should be done in a general way that can be used everywhere.
>
>    [Note that I doubt that "if".]

Hmmm I suppose that is a good point. The main issue with SPI flash is
that it is extremely slow, and writing a few MB can take a minute or
so. The 'sf update' command was intended to do a smart update, and the
progress is useful for that. Other storage types are not so bad.

Yes it makes sense (if we want this sort of feature) to make it more
general feature so that it can be used by all devices. I think it
could be done, and then enabled by a CONFIG. We could perhaps make it
so that the progress info never appears unless the operation takes at
least a few seconds.

>
> 2) The code size hurts (at least in some cases). Such code should be
>    optional. Please make it configurable.

OK see above. I will take a look.

Regards,
Simon

>
>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
> A witty saying proves nothing, but saying  something  pointless  gets
> people's attention.

[U-Boot] [PATCH 1/2] spi: Add progress percentage and write speed to `sf update`

[Scott Wood]

On 12/19/2012 05:20:07 PM, Simon Glass wrote:
> Hi Wolfgang,
> 
> On Wed, Dec 19, 2012 at 3:10 PM, Wolfgang Denk <wd@denx.de> wrote:
> > Dear Simon Glass,
> >
> > In message <1348878482-1730-1-git-send-email-sjg@chromium.org> you  
> wrote:
> >> From: James Miller <jamesmiller@chromium.org>
> >>
> >> Output a progress update only at most 10 times per second, to avoid
> >> saturating (and waiting on) the console. Make the summary line
> >> to fit on a single line. Make sure that cursor sits at the end of
> >> each update line instead of the beginning.
> >>
> >> Sample output:
> >>
> >> SF: Detected W25Q32 with page size 4 KiB, total 4 MiB
> >> Update SPI
> >> 1331200 bytes written, 2863104 bytes skipped in 21.912s, speed  
> 199728 B/s
> >
> > I dislike making commands more verbose then needed, or helpful.  Of
> > course the latter may be considered a matter of taste, but first of
> > all you also add code size here for questionable benefit.
> >
> > I object against this patch:
> >
> > 1) I cannot see what is so special in the "sf" command that it needs
> >    such handling, while commands accessing NOR or NAND flash or
> >    SDCard or any other storage devices don't.
> >
> >    If there is an agreement that this feature should be added, then  
> it
> >    should be done in a general way that can be used everywhere.
> >
> >    [Note that I doubt that "if".]
> 
> Hmmm I suppose that is a good point. The main issue with SPI flash is
> that it is extremely slow, and writing a few MB can take a minute or
> so. The 'sf update' command was intended to do a smart update, and the
> progress is useful for that. Other storage types are not so bad.

NOR can be pretty slow as well -- and it does have a progress indicator  
in U-Boot (albeit a simpler one).

NAND has a progress meter on erase, and for larger transfers it could  
probably use one on read/write as well.

-Scott

[U-Boot] [PATCH 1/2] spi: Add progress percentage and write speed to `sf update`

[Wolfgang Denk]

Dear Scott Wood,

In message <1355960533.12062.16@snotra> you wrote:
>
> > Hmmm I suppose that is a good point. The main issue with SPI flash is
> > that it is extremely slow, and writing a few MB can take a minute or
> > so. The 'sf update' command was intended to do a smart update, and the
> > progress is useful for that. Other storage types are not so bad.
> 
> NOR can be pretty slow as well -- and it does have a progress indicator  
> in U-Boot (albeit a simpler one).

Actually even this should be configurable.

Especially the spinning wheel used on some boards is a mess.


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de

"What happened to the crewman?"     "The M-5 computer  needed  a  new
power source, the crewman merely got in the way."
	-- Kirk and Dr. Richard Daystrom, "The Ultimate Computer",
	   stardate 4731.3.