Patchwork [U-Boot] cmd_time: add time command

login
register
mail settings
Submitter Che-liang Chiou
Date Oct. 5, 2011, 7:09 a.m.
Message ID <1317798555-2720-1-git-send-email-clchiou@chromium.org>
Download mbox | patch
Permalink /patch/117745/
State Changes Requested
Headers show

Comments

Che-liang Chiou - Oct. 5, 2011, 7:09 a.m.
The 'time' command runs and reports execution time of commands.

Sameple usage:
--------------------
u-boot# time crc 0x1000 1000
CRC32 for 00001000 ... 00001fff ==> ae94dc4b

time: 0.004 seconds, 4 ticks
--------------------

Signed-off-by: Che-Liang Chiou <clchiou@chromium.org>
---
 README                   |    1 +
 common/Makefile          |    1 +
 common/cmd_time.c        |   84 ++++++++++++++++++++++++++++++++++++++++++++++
 include/config_cmd_all.h |    1 +
 4 files changed, 87 insertions(+), 0 deletions(-)
 create mode 100644 common/cmd_time.c
Mike Frysinger - Oct. 5, 2011, 5:24 p.m.
On Wednesday 05 October 2011 03:09:15 Che-Liang Chiou wrote:
> The 'time' command runs and reports execution time of commands.

cool

> Sameple usage:

typo: Sample

> --- /dev/null
> +++ b/common/cmd_time.c
>
> +static void report_time(unsigned long int cycles)
> +{
> +#ifdef CONFIG_SYS_HZ

CONFIG_SYS_HZ is required to be defined, so please drop this ifdef check

> +	unsigned long int minutes, seconds, milliseconds;
> +	unsigned long int total_seconds, remainder;
> +
> +	total_seconds = cycles / CONFIG_SYS_HZ;
> +	remainder = cycles % CONFIG_SYS_HZ;
> +	minutes = total_seconds / 60;
> +	seconds = total_seconds % 60;
> +	/* approximate millisecond value */
> +	milliseconds = (remainder * 1000 + CONFIG_SYS_HZ / 2) / CONFIG_SYS_HZ;
> +
> +	printf("time:");
> +	if (minutes)
> +		printf(" %lu minutes,", minutes);
> +	printf(" %lu.%03lu seconds, %lu ticks\n",
> +			seconds, milliseconds, cycles);

looks like this could use the new timer api that Graeme is throwing around

> +int do_time(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])

static

> +	cmd_tbl_t *target_cmdtp = NULL;

const

> +	if (argc == 1) {
> +		printf("no command provided\n");
> +		return 1;
> +	}

return cmd_usage(cmdtp);

> +	if (!target_cmdtp) {
> +		printf("command not found: %s\n", argv[1]);
> +		return 1;
> +	}

shells typically display:
	%s: command not found

> +	if (target_argc > target_cmdtp->maxargs) {
> +		printf("maxarags exceeded: %d > %d\n", target_argc,
> +				target_cmdtp->maxargs);
> +		return 1;
> +	}
> ...
> +	retval = target_cmdtp->cmd(target_cmdtp, 0, target_argc, argv + 1);

hmm, this looks like we should add a helper like run_command() and put this 
logic there ...

> +	putc('\n');
> +	report_time(cycles);

why not just integrate that \n into the report_time() code ?
-mike
Che-liang Chiou - Oct. 6, 2011, 8:38 a.m.
Hi Mike,

Thanks for comments. Reply inlined.

On Thu, Oct 6, 2011 at 1:24 AM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Wednesday 05 October 2011 03:09:15 Che-Liang Chiou wrote:
>> The 'time' command runs and reports execution time of commands.
>
> cool
>
>> Sameple usage:
>
> typo: Sample
Done.
>
>> --- /dev/null
>> +++ b/common/cmd_time.c
>>
>> +static void report_time(unsigned long int cycles)
>> +{
>> +#ifdef CONFIG_SYS_HZ
>
> CONFIG_SYS_HZ is required to be defined, so please drop this ifdef check
>
Done.
>> +     unsigned long int minutes, seconds, milliseconds;
>> +     unsigned long int total_seconds, remainder;
>> +
>> +     total_seconds = cycles / CONFIG_SYS_HZ;
>> +     remainder = cycles % CONFIG_SYS_HZ;
>> +     minutes = total_seconds / 60;
>> +     seconds = total_seconds % 60;
>> +     /* approximate millisecond value */
>> +     milliseconds = (remainder * 1000 + CONFIG_SYS_HZ / 2) / CONFIG_SYS_HZ;
>> +
>> +     printf("time:");
>> +     if (minutes)
>> +             printf(" %lu minutes,", minutes);
>> +     printf(" %lu.%03lu seconds, %lu ticks\n",
>> +                     seconds, milliseconds, cycles);
>
> looks like this could use the new timer api that Graeme is throwing around
>
Exactly. Add TODO comments.
>> +int do_time(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>
> static
>
Done.
>> +     cmd_tbl_t *target_cmdtp = NULL;
>
> const
>
I guess I cannot change it to a const pointer. cmdtp->cmd() signature
requires a non-const pointer to cmdtp.
>> +     if (argc == 1) {
>> +             printf("no command provided\n");
>> +             return 1;
>> +     }
>
> return cmd_usage(cmdtp);
>
Done.
>> +     if (!target_cmdtp) {
>> +             printf("command not found: %s\n", argv[1]);
>> +             return 1;
>> +     }
>
> shells typically display:
>        %s: command not found
>
Done.
>> +     if (target_argc > target_cmdtp->maxargs) {
>> +             printf("maxarags exceeded: %d > %d\n", target_argc,
>> +                             target_cmdtp->maxargs);
>> +             return 1;
>> +     }
>> ...
>> +     retval = target_cmdtp->cmd(target_cmdtp, 0, target_argc, argv + 1);
>
> hmm, this looks like we should add a helper like run_command() and put this
> logic there ...
>
I agree. I moved these codes to a separate function. It can be merged
with run_command() if necessary after the timer API is landed.
>> +     putc('\n');
>> +     report_time(cycles);
>
> why not just integrate that \n into the report_time() code ?
Done.
> -mike
>

Regards,
Che-Liang
Mike Frysinger - Oct. 6, 2011, 6:51 p.m.
On Thu, Oct 6, 2011 at 04:38, Che-liang Chiou wrote:
> On Thu, Oct 6, 2011 at 1:24 AM, Mike Frysinger wrote:
>> On Wednesday 05 October 2011 03:09:15 Che-Liang Chiou wrote:
>>> +     cmd_tbl_t *target_cmdtp = NULL;
>>
>> const
>
> I guess I cannot change it to a const pointer. cmdtp->cmd() signature
> requires a non-const pointer to cmdtp.

ah, yeah, that's something i've been meaning to fix for a while
-mike

Patch

diff --git a/README b/README
index 91b6695..d6017e5 100644
--- a/README
+++ b/README
@@ -769,6 +769,7 @@  The following options need to be configured:
 		CONFIG_CMD_SOURCE	  "source" command Support
 		CONFIG_CMD_SPI		* SPI serial bus support
 		CONFIG_CMD_TFTPSRV	* TFTP transfer in server mode
+		CONFIG_CMD_TIME		* run command and report execution time
 		CONFIG_CMD_USB		* USB support
 		CONFIG_CMD_CDP		* Cisco Discover Protocol support
 		CONFIG_CMD_FSL		* Microblaze FSL support
diff --git a/common/Makefile b/common/Makefile
index 371a0d9..fdc4206 100644
--- a/common/Makefile
+++ b/common/Makefile
@@ -147,6 +147,7 @@  COBJS-$(CONFIG_CMD_SPI) += cmd_spi.o
 COBJS-$(CONFIG_CMD_SPIBOOTLDR) += cmd_spibootldr.o
 COBJS-$(CONFIG_CMD_STRINGS) += cmd_strings.o
 COBJS-$(CONFIG_CMD_TERMINAL) += cmd_terminal.o
+COBJS-$(CONFIG_CMD_TIME) += cmd_time.o
 COBJS-$(CONFIG_SYS_HUSH_PARSER) += cmd_test.o
 COBJS-$(CONFIG_CMD_TSI148) += cmd_tsi148.o
 COBJS-$(CONFIG_CMD_UBI) += cmd_ubi.o
diff --git a/common/cmd_time.c b/common/cmd_time.c
new file mode 100644
index 0000000..3c49615
--- /dev/null
+++ b/common/cmd_time.c
@@ -0,0 +1,84 @@ 
+/*
+ * Copyright (c) 2011 The Chromium OS Authors.
+ * See file CREDITS for list of people who contributed to this
+ * project.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+
+#include <common.h>
+#include <command.h>
+
+static void report_time(unsigned long int cycles)
+{
+#ifdef CONFIG_SYS_HZ
+	unsigned long int minutes, seconds, milliseconds;
+	unsigned long int total_seconds, remainder;
+
+	total_seconds = cycles / CONFIG_SYS_HZ;
+	remainder = cycles % CONFIG_SYS_HZ;
+	minutes = total_seconds / 60;
+	seconds = total_seconds % 60;
+	/* approximate millisecond value */
+	milliseconds = (remainder * 1000 + CONFIG_SYS_HZ / 2) / CONFIG_SYS_HZ;
+
+	printf("time:");
+	if (minutes)
+		printf(" %lu minutes,", minutes);
+	printf(" %lu.%03lu seconds, %lu ticks\n",
+			seconds, milliseconds, cycles);
+#else
+	printf("CONFIG_SYS_HZ not defined\n");
+	printf("time: %lu ticks\n", cycles);
+#endif
+}
+
+int do_time(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
+{
+	const int target_argc = argc - 1;
+	int retval = 0;
+	unsigned long int cycles = 0;
+	cmd_tbl_t *target_cmdtp = NULL;
+
+	if (argc == 1) {
+		printf("no command provided\n");
+		return 1;
+	}
+
+	target_cmdtp = find_cmd(argv[1]);
+	if (!target_cmdtp) {
+		printf("command not found: %s\n", argv[1]);
+		return 1;
+	}
+	if (target_argc > target_cmdtp->maxargs) {
+		printf("maxarags exceeded: %d > %d\n", target_argc,
+				target_cmdtp->maxargs);
+		return 1;
+	}
+
+	cycles = get_timer_masked();
+	retval = target_cmdtp->cmd(target_cmdtp, 0, target_argc, argv + 1);
+	cycles = get_timer_masked() - cycles;
+
+	putc('\n');
+	report_time(cycles);
+
+	return retval;
+}
+
+U_BOOT_CMD(time, CONFIG_SYS_MAXARGS, 0, do_time,
+		"run commands and summarize execution time",
+		"command [args...]\n");
diff --git a/include/config_cmd_all.h b/include/config_cmd_all.h
index 9716f9c..181674b 100644
--- a/include/config_cmd_all.h
+++ b/include/config_cmd_all.h
@@ -82,6 +82,7 @@ 
 #define CONFIG_CMD_SOURCE	/* "source" command support	*/
 #define CONFIG_CMD_SPI		/* SPI utility			*/
 #define CONFIG_CMD_TERMINAL	/* built-in Serial Terminal	*/
+#define CONFIG_CMD_TIME		/* run command and report execution time */
 #define CONFIG_CMD_UBI		/* UBI Support			*/
 #define CONFIG_CMD_UBIFS	/* UBIFS Support		*/
 #define CONFIG_CMD_UNIVERSE	/* Tundra Universe Support	*/