[U-Boot] [PATCH 1/2] spi: Add progress percentage and write speed to `sf update`
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; }
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@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 at `offset'" + SF_TEST_HELP );
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_TESTDefine this option to include a destructive SPI flashtest ('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@chromium.org): Support cmdline parameters for these */
Lets just add that now? :) Thanks.
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_TESTDefine this option to include a destructive SPI flashtest ('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@chromium.org): Support cmdline parameters for these */Lets just add that now? :) Thanks.
Fair enough, will do.
Regards, Simon
-- Tom
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_TESTDefine this option to include a destructive SPI flashtest ('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@chromium.org): Support cmdline parameters for these */Lets just add that now? :) Thanks.
Fair enough, will do.
Thanks!
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@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
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.
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
-----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
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.
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
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.
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@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
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.
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@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Hi Jagan,
On Fri, Dec 21, 2012 at 12:46 AM, Jagan Teki jagannadh.teki@gmail.comwrote:
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@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
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
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
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:
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.
- 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@denx.de A witty saying proves nothing, but saying something pointless gets people's attention.
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:
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
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
participants (5)
-
Jagan Teki -
Scott Wood -
Simon Glass -
Tom Rini -
Wolfgang Denk