Add time measurement command to u-boot

Add time measurement command to u-boot

The command "time" takes a command as its argument, executes the command, and reports how much time the command has used.

Example usage:
------------------------------------------------------------
CrOS> time sleep 1

time: 1.000 seconds, 1000 ticks
CrOS> time sleep 10

time: 10.000 seconds, 10000 ticks
------------------------------------------------------------

R=robotboy@chromium.org,sjg@chromium.org
BUG=none
TEST=see above

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=e14cdef

[Che-Liang Chiou, 2011-03-31 11:08:08]

Do you think I should better send two CLs, one for add this to u-boot, and one
for enable this command in include/configs/chromeos/developer.h?

[sjg, 2011-03-31 13:45:45]

Please see comments. Yes to your question please. In fact once Anton pushes the
change to a separate repository for U-Boot config you will have to use separate
CLs anyway.

Thanks for adding this command - I think it will be useful for manual testing.
We should send this upstream.

http://codereview.chromium.org/6670118/diff/1/common/cmd_time.c
File common/cmd_time.c (right):

http://codereview.chromium.org/6670118/diff/1/common/cmd_time.c#newcode11
common/cmd_time.c:11: /* time - run command and report its run time */
run a command and ...

http://codereview.chromium.org/6670118/diff/1/common/cmd_time.c#newcode26
common/cmd_time.c:26: /*
Can you just use something like:

milliseconds = cycles - 60 * (seconds + minutes * 60)

This code seems overly complicated to me. If there is some reason for this that
I don't understand please let me know.

http://codereview.chromium.org/6670118/diff/1/common/cmd_time.c#newcode55
common/cmd_time.c:55: printf("time:\t%lum%lu.%03lus\n", minutes, seconds,
milliseconds);
It might be better to omit minutes if it is 0, and perhaps even omit seconds if
zero.

http://codereview.chromium.org/6670118/diff/1/common/cmd_time.c#newcode60
common/cmd_time.c:60: printf("cycles:\t%lu\n", cycles);
Perhaps put this on the same line as the 'time' output?

time: 4345 ticks, 4.345s

http://codereview.chromium.org/6670118/diff/1/common/cmd_time.c#newcode66
common/cmd_time.c:66: 
remove blank line

http://codereview.chromium.org/6670118/diff/1/common/cmd_time.c#newcode72
common/cmd_time.c:72: printf("no command provided\n");
Linux time prints elapsed time anyway in this case. Should you do the same?

http://codereview.chromium.org/6670118/diff/1/common/cmd_time.c#newcode77
common/cmd_time.c:77: 
remove blank line

http://codereview.chromium.org/6670118/diff/1/common/cmd_time.c#newcode91
common/cmd_time.c:91: 
remove blank line

http://codereview.chromium.org/6670118/diff/1/common/cmd_time.c#newcode92
common/cmd_time.c:92: cycles = get_timer_masked();
cycles should perhaps be time_ms?

http://codereview.chromium.org/6670118/diff/1/common/cmd_time.c#newcode103
common/cmd_time.c:103: "run command and report its run time",
run a command...

http://codereview.chromium.org/6670118/diff/1/include/configs/chromeos/develo...
File include/configs/chromeos/developer.h (right):

http://codereview.chromium.org/6670118/diff/1/include/configs/chromeos/develo...
include/configs/chromeos/developer.h:21: #define CONFIG_CMD_GETTIME
What is GETTIME?

[Che-Liang Chiou, 2011-04-04 09:24:40]

Hi Simon,

Thanks for comments. Replies are inlined. Please take a look.

http://codereview.chromium.org/6670118/diff/1/common/cmd_time.c
File common/cmd_time.c (right):

http://codereview.chromium.org/6670118/diff/1/common/cmd_time.c#newcode11
common/cmd_time.c:11: /* time - run command and report its run time */
On 2011/03/31 13:45:45, sjg wrote:
> run a command and ...

Done.

http://codereview.chromium.org/6670118/diff/1/common/cmd_time.c#newcode26
common/cmd_time.c:26: /*
On 2011/03/31 13:45:45, sjg wrote:
> Can you just use something like:
> 
> milliseconds = cycles - 60 * (seconds + minutes * 60)
I guess you were trying to write
  milliseconds = cycles - 1000 * (seconds + minutes * 60)
because 1 second = 1000 milliseconds.

The formula implicitly assumes that 1 cycle = 1 millisecond (i.e., CONFIG_SYS_HZ
= 1000), but it is not always the case.
> 
> This code seems overly complicated to me. If there is some reason for this
that
> I don't understand please let me know.

You are right, it is overly complex. I found a simpler and equally precise way
to approximate millisecond value.

http://codereview.chromium.org/6670118/diff/1/common/cmd_time.c#newcode55
common/cmd_time.c:55: printf("time:\t%lum%lu.%03lus\n", minutes, seconds,
milliseconds);
On 2011/03/31 13:45:45, sjg wrote:
> It might be better to omit minutes if it is 0, and perhaps even omit seconds
if
> zero.

Nice. I think it is a good idea to output differently to Unix's time command
since we are not a exact replica of it.

http://codereview.chromium.org/6670118/diff/1/common/cmd_time.c#newcode60
common/cmd_time.c:60: printf("cycles:\t%lu\n", cycles);
On 2011/03/31 13:45:45, sjg wrote:
> Perhaps put this on the same line as the 'time' output?
> 
> time: 4345 ticks, 4.345s

Done.

http://codereview.chromium.org/6670118/diff/1/common/cmd_time.c#newcode66
common/cmd_time.c:66: 
On 2011/03/31 13:45:45, sjg wrote:
> remove blank line

Done.

http://codereview.chromium.org/6670118/diff/1/common/cmd_time.c#newcode72
common/cmd_time.c:72: printf("no command provided\n");
On 2011/03/31 13:45:45, sjg wrote:
> Linux time prints elapsed time anyway in this case. Should you do the same?

I think we are not making a exact replica of Unix's time command in u-boot. To
make that clear for a user, I think we should output differently to Unit's time
command. What do you think?

http://codereview.chromium.org/6670118/diff/1/common/cmd_time.c#newcode77
common/cmd_time.c:77: 
On 2011/03/31 13:45:45, sjg wrote:
> remove blank line

Done.

http://codereview.chromium.org/6670118/diff/1/common/cmd_time.c#newcode91
common/cmd_time.c:91: 
On 2011/03/31 13:45:45, sjg wrote:
> remove blank line

Done.

http://codereview.chromium.org/6670118/diff/1/common/cmd_time.c#newcode92
common/cmd_time.c:92: cycles = get_timer_masked();
On 2011/03/31 13:45:45, sjg wrote:
> cycles should perhaps be time_ms?

cycles will be identical to milliseconds only if CONFIG_SYS_HZ is 1000 (though
it usually is).

http://codereview.chromium.org/6670118/diff/1/common/cmd_time.c#newcode103
common/cmd_time.c:103: "run command and report its run time",
On 2011/03/31 13:45:45, sjg wrote:
> run a command...

Done.

[sjg, 2011-04-05 01:21:44]

Looking better! Hoping to simplify further though.

http://codereview.chromium.org/6670118/diff/5001/common/cmd_time.c
File common/cmd_time.c (right):

http://codereview.chromium.org/6670118/diff/5001/common/cmd_time.c#newcode61
common/cmd_time.c:61: milliseconds = guess = remainder * 1000 / CONFIG_SYS_HZ;
I think it is good enough if you just stop here. Do we really care about an
error of 1ms on something printed on the console? If more accuracy is required,
then the best answer is to repeat the test multiple times.

Yes this code is better than last time but overly clever I think! Perhaps you
could just round it?

(remainder * 1000 + CONFIG_SYS_HZ / 2) / CONFIG_SYS_HZ.

http://codereview.chromium.org/6670118/diff/5001/common/cmd_time.c#newcode84
common/cmd_time.c:84: printf(" %lu minutes and", minutes);
It might be easier for a test system to parse if you leave out the words 'and'
and 'or' and just use a comma in each case.

http://codereview.chromium.org/6670118/diff/5001/common/cmd_time.c#newcode102
common/cmd_time.c:102: return 128;
What is the significance of 128? Is it argument error? Is there a constant
somewhere for this?

[Che-Liang Chiou, 2011-04-06 08:48:40]

Inlined. PTAL.

http://codereview.chromium.org/6670118/diff/5001/common/cmd_time.c
File common/cmd_time.c (right):

http://codereview.chromium.org/6670118/diff/5001/common/cmd_time.c#newcode61
common/cmd_time.c:61: milliseconds = guess = remainder * 1000 / CONFIG_SYS_HZ;
On 2011/04/05 01:21:44, sjg wrote:
> I think it is good enough if you just stop here. Do we really care about an
> error of 1ms on something printed on the console? If more accuracy is
required,
> then the best answer is to repeat the test multiple times.
> 
> Yes this code is better than last time but overly clever I think! Perhaps you
> could just round it?
> 
> (remainder * 1000 + CONFIG_SYS_HZ / 2) / CONFIG_SYS_HZ.

Thanks. This is better.

http://codereview.chromium.org/6670118/diff/5001/common/cmd_time.c#newcode84
common/cmd_time.c:84: printf(" %lu minutes and", minutes);
On 2011/04/05 01:21:44, sjg wrote:
> It might be easier for a test system to parse if you leave out the words 'and'
> and 'or' and just use a comma in each case.

Done.

http://codereview.chromium.org/6670118/diff/5001/common/cmd_time.c#newcode102
common/cmd_time.c:102: return 128;
On 2011/04/05 01:21:44, sjg wrote:
> What is the significance of 128? Is it argument error? Is there a constant
> somewhere for this?

I thought that we might be able to know either it was an internal error or a
target command failure by checking return value == 128 because 128 was an
unusual value that the target command would not return.

On a second thought, checking return value with 128 was fragile. So I think we
should just return 1.

[robotboy, 2011-04-06 21:24:11]

LGTM, thanks.

[sjg, 2011-04-06 21:40:05]

LGTM2 yes this is great thanks. Do you want to try upstreaming this? - Simon

On Wed, Apr 6, 2011 at 2:24 PM,  <robotboy@chromium.org> wrote:
> LGTM, thanks.
>
>
> http://codereview.chromium.org/6670118/
>

[Che-Liang Chiou, 2011-04-07 02:41:10]

Yes, I want to upstream this.

On 2011/04/06 21:40:05, sjg wrote:
> LGTM2 yes this is great thanks. Do you want to try upstreaming this? - Simon
> 
> On Wed, Apr 6, 2011 at 2:24 PM,  <mailto:robotboy@chromium.org> wrote:
> > LGTM, thanks.
> >
> >
> > http://codereview.chromium.org/6670118/
> >