| Message ID | 1348878482-1730-1-git-send-email-sjg@chromium.org |
|---|---|
| State | Accepted, archived |
| Delegated to: | Tom Rini |
| Headers | show |
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 -----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-----
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 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
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
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@denx.de > A witty saying proves nothing, but saying something pointless gets > people's attention.
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.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@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
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; }