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

login
register
mail settings
Submitter Simon Glass
Date Sept. 29, 2012, 12:28 a.m.
Message ID <1348878482-1730-1-git-send-email-sjg@chromium.org>
Download mbox | patch
Permalink /patch/187915/
State Accepted, archived
Delegated to: Tom Rini
Headers show

Comments

Simon Glass - Sept. 29, 2012, 12:28 a.m.
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(-)
Tom Rini - Dec. 19, 2012, 8:43 p.m.
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.
Simon Glass - Dec. 19, 2012, 8:46 p.m.
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
Tom Rini - Dec. 19, 2012, 8:59 p.m.
-----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-----
Tom Rini - Dec. 19, 2012, 10:59 p.m.
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.
Wolfgang Denk - Dec. 19, 2012, 11:10 p.m.
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
Wolfgang Denk - Dec. 19, 2012, 11:14 p.m.
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
Simon Glass - Dec. 19, 2012, 11:20 p.m.
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.
Tom Rini - Dec. 20, 2012, 1:03 a.m.
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.
Simon Glass - Dec. 20, 2012, 1:18 a.m.
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
>
Tom Rini - Dec. 20, 2012, 3:04 p.m.
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.
Jagannadha Sutradharudu Teki - Dec. 21, 2012, 8:46 a.m.
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
Simon Glass - Dec. 21, 2012, 6:21 p.m.
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
>
Wolfgang Denk - Dec. 21, 2012, 7:52 p.m.
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

Patch

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;
 }