spi: Add progress percentage and write speed to `sf update`
PATCH 1/2 spi: Add progress percentage and write speed to `sf update`
From: James Miller <[email protected]>
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
--- common/cmd_sf.c | 41 +++++++++++++++++++++++++++++++++++++++-- 1 files changed, 39 insertions(+), 2 deletions(-)
common/cmd_sf.c +39 -2
@@ -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 ; }
Signature · 1 line
1.7.7.3
PATCH 2/2 spi: Add SPI flash test
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.
--- README | 5 ++ common/cmd_sf.c | 160 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 165 insertions(+), 0 deletions(-)
README +5 -0
@@ -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
common/cmd_sf.c +160 -0
188 lines not on this page · raw message
Signature · 1 line
1.7.7.3
PATCH 2/2 spi: Add SPI flash test
Quoted 19 lines · 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_TESTto enable it.Signed-off-by Simon Glass <[email protected]>--- README | 5 ++ common/cmd_sf.c | 160 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 165 insertions(+), 0 deletions(-)README +0
-0
README The following options need to be configured: · line 2316
Quoted 8 lines
README +5
-0@@ -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]
README The following options need to be configured: · line 2319
Quoted 3 lines
README +3
-0+static int do_spi_flash_test(void) +{ + /* TODO(sjg at chromium.org): Support cmdline parameters for these */
Lets just add that now? :) Thanks.
Signature · 8 lines
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>
PATCH 2/2 spi: Add SPI flash test
Hi Tom,
Quoted 24 lines · Tom Rini
Quoted 22 lines · 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_TESTto enable it.Signed-off-by Simon Glass <[email protected]>--- README | 5 ++ common/cmd_sf.c | 160 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 165 insertions(+), 0 deletions(-)README +0
-0@@ -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
Quoted 7 lines
README The following options need to be configured: · line 2316
Quoted 5 lines
README +5
-0+ 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?
Quoted 7 lines
[snip]
README
Quoted 3 lines
README +3
-0+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
Quoted 3 lines
--
Tom
PATCH 2/2 spi: Add SPI flash test
Quoted 35 lines · Simon Glass
Hi Tom,
Quoted 24 lines · Tom Rini
Quoted 22 lines · 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_TESTto enable it.Signed-off-by Simon Glass <[email protected]>--- README | 5 ++ common/cmd_sf.c | 160 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 165 insertions(+), 0 deletions(-)README +0
-0@@ -2309,6 +2309,11 @@ The following options need to be configured: CONFIG_SF_DEFAULT_MODE (see include/spi.h) CONFIG_SF_DEFAULT_SPEED in HzQuoted 7 lines
README The following options need to be configured: · line 2316
Quoted 5 lines
README +5
-0+ 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.
Quoted 1 line
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.
Quoted 8 lines
Quoted 6 lines
[snip]
README
Quoted 3 lines
README +3
-0+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!
Signature · 8 lines
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>
PATCH 2/2 spi: Add SPI flash test
In reply to Simon GlassDear Simon Glass,
Quoted 7 lines · In message <[email protected]> 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_TESTto 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 <[email protected]>
PATCH 1/2 spi: Add progress percentage and write speed to `sf update`
In reply to Simon GlassQuoted 13 lines · Simon Glass
From: James Miller <[email protected]>
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
Quoted 6 lines
time: 21.919 seconds, 21919 ticks
Skipping verifySigned-off-by Simon Glass <[email protected]>Signed-off-by James Miller <[email protected]>Signed-off-by Taylor Hutt <[email protected]>
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.
Signature · 8 lines
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>
PATCH 1/2 spi: Add progress percentage and write speed to `sf update`
Hi Tom,
Quoted 18 lines · Tom Rini
Quoted 16 lines · Simon Glass
From: James Miller <[email protected]>
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/stime: 21.919 seconds, 21919 ticks
Skipping verify
Quoted 6 lines
Quoted 3 lines
Signed-off-by Simon Glass <[email protected]>Signed-off-by James Miller <[email protected]>Signed-off-by Taylor Hutt <[email protected]>With ELDK 4.2:
cmd_sf.c:80: warning: type qualifiers ignored on function return type
Interesting...
Quoted 7 lines
Unless I'm missing something, this is the right fix:
(quoted diff) +1
-1-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
Quoted 3 lines
--
Tom
PATCH 1/2 spi: Add progress percentage and write speed to `sf update`
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Quoted 34 lines · Simon Glass
Hi Tom,
Quoted 15 lines · Tom Rini
Quoted 13 lines · Simon Glass
From: James Miller <[email protected]>
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
Quoted 8 lines
Quoted 5 lines
time: 21.919 seconds, 21919 ticks Skipping verify
Signed-off-by Simon Glass <[email protected]> Signed-off-by:James Miller <[email protected]> Signed-off-by: Taylor Hutt <[email protected]>
With ELDK 4.2: cmd_sf.c:80: warning: type qualifiers ignored on function return type
Interesting...
Quoted 5 lines
Unless I'm missing something, this is the right fix: -static const ulong
bytes_per_second(unsigned int len, ulong start_ms)+static ulongbytes_per_second(unsigned int len, ulong start_ms)
Quoted 5 lines
Quoted 2 lines
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-----
PATCH 1/2 spi: Add progress percentage and write speed to `sf update`
Quoted 41 lines · Tom Rini
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1Quoted 37 lines · Simon Glass
Hi Tom,
Quoted 15 lines · Tom Rini
Quoted 13 lines · Simon Glass
From: James Miller <[email protected]>
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
Quoted 8 lines
Quoted 5 lines
time: 21.919 seconds, 21919 ticks Skipping verify
Signed-off-by Simon Glass <[email protected]> Signed-off-by:James Miller <[email protected]> Signed-off-by: Taylor Hutt <[email protected]>
With ELDK 4.2: cmd_sf.c:80: warning: type qualifiers ignored on function return type
Interesting...
Quoted 7 lines
Unless I'm missing something, this is the right fix: -static const ulong
bytes_per_second(unsigned int len, ulong start_ms)+static ulongbytes_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.
Quoted 6 lines
Quoted 2 lines
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.
Signature · 8 lines
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>
PATCH 1/2 spi: Add progress percentage and write speed to `sf update`
Dear Tom Rini,
Quoted 1 line · In message <20121219225945.GF14589@bill-the-cat> you wrote:
...
Quoted 1 line
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
Signature · 6 lines
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
PATCH 1/2 spi: Add progress percentage and write speed to `sf update`
Quoted 5 lines · Wolfgang Denk
Dear Tom Rini,
Quoted 1 line · In message <20121219225945.GF14589@bill-the-cat> you wrote:
...
Quoted 8 lines
Quoted 1 line
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.
Signature · 8 lines
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>
PATCH 1/2 spi: Add progress percentage and write speed to `sf update`
Hi Tom,
Quoted 15 lines · Tom Rini
Quoted 5 lines · Wolfgang Denk
Dear Tom Rini,
Quoted 1 line · In message <20121219225945.GF14589@bill-the-cat> you wrote:
...
Quoted 8 lines
Quoted 1 line
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.
Quoted 5 lines
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
Quoted 9 lines
--
Tom_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
PATCH 1/2 spi: Add progress percentage and write speed to `sf update`
Quoted 18 lines · Simon Glass
Hi Tom,
Quoted 15 lines · Tom Rini
Quoted 13 lines · Wolfgang Denk
Dear Tom Rini,
Quoted 1 line · In message <20121219225945.GF14589@bill-the-cat> you wrote:
...
Quoted 1 line
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.
Quoted 8 lines
Quoted 5 lines
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.
Quoted 4 lines
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.
Signature · 8 lines
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>
PATCH 1/2 spi: Add progress percentage and write speed to `sf update`
In reply to Simon GlassHi Simon,
Quoted 24 lines · Simon Glass
Hi Tom,
Quoted 15 lines · Tom Rini
Quoted 5 lines · Wolfgang Denk
Dear Tom Rini,
Quoted 1 line · In message <20121219225945.GF14589@bill-the-cat> you wrote:
...
Quoted 8 lines
Quoted 1 line
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.
Quoted 5 lines
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.
Quoted 8 lines
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.
Quoted 16 lines
Regards,
SimonQuoted 9 lines
--
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
PATCH 1/2 spi: Add progress percentage and write speed to `sf update`
Hi Jagan,
Quoted 36 lines · Jagan Teki
Hi Simon,
Quoted 24 lines · Simon Glass
Hi Tom,
Quoted 15 lines · Tom Rini
Quoted 5 lines · Wolfgang Denk
Dear Tom Rini,
Quoted 1 line · In message <20121219225945.GF14589@bill-the-cat> you wrote:
...
Quoted 8 lines
Quoted 1 line
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.
Quoted 5 lines
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.
Quoted 8 lines
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.
Quoted 4 lines
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
Quoted 1 line
=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
Quoted 22 lines
Thanks,
Jagan.Quoted 16 lines
Regards,
SimonQuoted 9 lines
--
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
PATCH 1/2 spi: Add progress percentage and write speed to `sf update`
Dear Simon,
Quoted 3 lines · In message <CAPnjgZ35OaUPvMcc7Z_GHL4awWkxVZPHoOpEQjDLe0haP4e7cw@mail.gmail.com>…
Yes there seems to be a plan. Perhaps I will sketch out a few ideas so that people can comment:
Thanks!
Quoted 5 lines
Add two environment variables:
verbose=0|1 - if this is 0, then commands complete silently as now. If
Quoted 1 line
=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.
Quoted 3 lines
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).
Quoted 2 lines
We also need a CONFIG for each to enable it, like perhaps
CONFIG_SYS_VERBOSEandCONFIG_SYS_PROGRESS.
This should be no _SYS, as it should be user-selectable.
Quoted 2 lines
/* may need to record column position of last message so we can clear itat the end */
Clear? Just print a '\n' and continue on a new line, please.
Thanks.
Best regards,
Wolfgang Denk
Signature · 4 lines
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!
PATCH 1/2 spi: Add progress percentage and write speed to `sf update`
In reply to Simon GlassDear Simon Glass,
Quoted 7 lines · In message <[email protected]> you wrote:
From: James Miller <[email protected]>
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.
Quoted 5 lines
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
Signature · 5 lines
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.
PATCH 1/2 spi: Add progress percentage and write speed to `sf update`
Hi Wolfgang,
Quoted 22 lines · Wolfgang Denk
Dear Simon Glass,
Quoted 7 lines · In message <[email protected]> you wrote:
From: James Miller <[email protected]>
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.
Quoted 5 lines
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/sI 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:
Quoted 8 lines
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.
Quoted 3 lines
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
Quoted 12 lines
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.
PATCH 1/2 spi: Add progress percentage and write speed to `sf update`
Quoted 33 lines · Simon Glass
Hi Wolfgang,
Quoted 3 lines · Wolfgang Denk
Dear Simon Glass,
In message <[email protected]> you
Quoted 7 lines · wrote:
Quoted 7 lines
From: James Miller <[email protected]>
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.
Quoted 5 lines
Quoted 5 lines
Sample output:
SF: Detected W25Q32 with page size 4 KiB, total 4 MiB Update SPI
1331200 bytes written, 2863104 bytes skipped in 21.912s, speed199728 B/s
Quoted 5 lines
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.
Quoted 7 lines
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
Quoted 8 lines
Quoted 3 lines
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
PATCH 1/2 spi: Add progress percentage and write speed to `sf update`
Dear Scott Wood,
Quoted 8 lines · In message <1355960533.12062.16@snotra> you wrote:
Quoted 5 lines
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
Signature · 8 lines
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.