-----Original Message----- From: sjg@google.com [mailto:sjg@google.com] On Behalf Of Simon Glass Sent: Thursday, March 10, 2016 3:09 AM To: Rajat Srivastava rajat.srivastava@nxp.com Cc: U-Boot Mailing List u-boot@lists.denx.de; Marek VaĊĦut marex@denx.de; Rajesh Bhagat rajesh.bhagat@nxp.com Subject: Re: [PATCH] usb: Add new command to regress USB devices
Hi Rajat,
On 9 March 2016 at 04:22, Rajat Srivastava rajat.srivastava@nxp.com wrote:
This patch adds a new 'usb regress' command, that can be used to regress test a USB device. It performs the following operations:
- starts the USB device
- performs read/write operations
- stops the USB device
- verifies the contents of read/write operations
Sample Output: => usb regress 81000000 82000000 32m regressing USB.. starting USB... USB0: Register 200017f NbrPorts 2 Starting the controller USB XHCI 1.00 scanning bus 0 for devices... 2 USB Device(s) found scanning usb for storage devices... 1 Storage Device(s) found USB write: device 0 block # 0, count 65536 ... 65536 blocks write: OK USB read: device 0 block # 0, count 65536 ... 65536 blocks read: OK stopping USB.. verifying data on addresses 0x81000000 and 0x82000000 Total of 65536 word(s) were the same
Signed-off-by: Rajat Srivastava rajat.srivastava@nxp.com Signed-off-by: Rajesh Bhagat rajesh.bhagat@nxp.com
common/cmd_usb.c | 174
++++++++++++++++++++++++++++++++++++++++++++++++++++ ++-
1 file changed, 173 insertions(+), 1 deletion(-)
Can you rebase to mainline? This file has been renamed.
Will take care v2.
diff --git a/common/cmd_usb.c b/common/cmd_usb.c index a540b42..25fdeab 100644 --- a/common/cmd_usb.c +++ b/common/cmd_usb.c @@ -20,6 +20,7 @@ #include <asm/unaligned.h> #include <part.h> #include <usb.h> +#include <mapmem.h>
#ifdef CONFIG_USB_STORAGE static int usb_stor_curr_dev = -1; /* current device */ @@ -616,6 +617,167 @@ static int usb_device_info(void) } #endif
+static unsigned long calc_blockcount(char * const size)
Can you put this function in lib/display_options.c? I suggest something that decodes a string and returns a value (i.e. it would return 1024 for K, not 2, since that assumes a block size).
The multiple check can go in cmd/usb.c
Will take care v2.
+{
unsigned long value, multiplier;int size_len = strlen(size);char unit;/* extract the unit of size passed */unit = size[size_len - 1];/* updating the source string to remove unit */size[size_len - 1] = '\0';value = simple_strtoul(size, NULL, 10);if (value <= 0) {printf("invalid size\n");return 0;}if (unit == 'G' || unit == 'g') {multiplier = 2 * 1024 * 1024;} else if (unit == 'M' || unit == 'm') {multiplier = 2 * 1024;} else if (unit == 'K' || unit == 'k') {multiplier = 2;} else if (unit == 'B' || unit == 'b') {if (value % 512 != 0) {printf("size can only be multiples of 512 bytes\n");return 0;}multiplier = 1;value /= 512;} else {printf("syntax mismatch\n");return 0;}return value * multiplier;+}
+static int usb_read_write_verify(unsigned long w_addr, unsigned long r_addr,
unsigned long+cnt) {
cmd_tbl_t *c;char str[3][16];char *ptr[4] = { "cmp", str[0], str[1], str[2] };c = find_cmd("cmp");if (!c) {printf("compare command not found\n");return -1;}printf("verifying data on addresses 0x%lx and 0x%lx\n", w_addr, r_addr);sprintf(str[0], "%lx", w_addr);sprintf(str[1], "%lx", r_addr);sprintf(str[2], "%lx", cnt);(c->cmd)(c, 0, 4, ptr);We shouldn't call U-Boot functions via the command line parsing.
Please can you refactor do_mem_cmp() to separate the command parsing from the memory comparison logic? Then you can call the latter directory.
AFAIU, we need to refactor do_mem_cmp to two functions and call mem_cmp function in our code. Please confirm.
return 0;+}
+static int do_usb_regress(int argc, char * const argv[])
Would 'usb datatest' be a better name?
How about renaming the existing "usb test" command to "usb hwtest" as it supports hardware tests. And add the new proposed command as "usb test" ? "usb test [dev] [port] [mode] - set USB 2.0 test mode\n" " (specify port 0 to indicate the device's upstream port)\n" " Available modes: J, K, S[E0_NAK], P[acket], F[orce_Enable]\n"
And it will also help to align the naming convention with "sf test". Please share your opinion.
+{
unsigned long loopcount, iteration;unsigned long w_addr, r_addr, cnt, n;unsigned long blk = 0;extern char usb_started;+#ifdef CONFIG_USB_STORAGE
block_dev_desc_t *stor_dev;+#endif
if (argc < 5 || argc > 6) {printf("syntax mismatch\n");return -1;}if (argc == 5)loopcount = 1;elseloopcount = simple_strtoul(argv[5], NULL, 10);if (loopcount <= 0) {printf("syntax mismatch\n");return -1;}cnt = calc_blockcount(argv[4]);if (cnt == 0)return -1;iteration = loopcount;while (loopcount--) {if (argc > 5)printf("\niteration #%lu\n\n", iteration -- loopcount);
/* start USB */if (usb_started) {printf("USB already started\n");} else {printf("starting USB...\n");do_usb_start();}if (!usb_started) {printf("USB did not start\n");return -1;}if (usb_stor_curr_dev < 0) {printf("no current device selected\nstopping USB...\n");usb_stop();return -1;}+#ifdef CONFIG_USB_STORAGE
If this is not defined, perhaps this command shouldn't be included at all?
Will take care v2. And add command definition in above flag.
/* write on USB from address (w_addr) of RAM */w_addr = simple_strtoul(argv[2], NULL, 16);Parse your arguments at the start, not in the loop.
Agreed. Will take care v2
printf("USB write: device %d block # %ld, count %ld ... ",usb_stor_curr_dev, blk, cnt);stor_dev = usb_stor_get_dev(usb_stor_curr_dev);n = stor_dev->block_write(usb_stor_curr_dev, blk, cnt,(ulong *)w_addr);printf("%ld blocks write: %s\n", n, (n == cnt) ?"OK" : "ERROR");if (n != cnt) {printf("aborting.. USB write failed\n");usb_stop();return -1;}/* read from USB and write on to address (r_addr) on RAM */r_addr = simple_strtoul(argv[3], NULL, 16);printf("USB read: device %d block # %ld, count %ld ... ",usb_stor_curr_dev, blk, cnt);stor_dev = usb_stor_get_dev(usb_stor_curr_dev);n = stor_dev->block_read(usb_stor_curr_dev, blk, cnt,(ulong *)r_addr);printf("%ld blocks read: %s\n", n, (n == cnt) ? "OK" : "ERROR");if (n != cnt) {printf("aborting.. USB read failed\n");usb_stop();return -1;I think this needs to be CMD_RET_FAILURE.
Agreed. Will take care v2
}+#endif
Can this test pass on sandbox?
Ok, We try to make its setup and share results.
/* stop USB */printf("stopping USB..\n");It would be good to display the average read and write transfer speed here. See 'sf test'.
Ok, we will refer "sf test" command" and add it in v2.
usb_stop();+#ifdef CONFIG_USB_STORAGE
/** verify the content written on USB and* content read from USB.*/if (usb_read_write_verify(w_addr, r_addr, cnt) == -1)return -1;+#endif
if (ctrlc())return -1;}return 0;+}
/**********************************************************************
- usb command intepreter
*/ @@ -656,6 +818,13 @@ static int do_usb(cmd_tbl_t *cmdtp, int flag, int argc, char
- const argv[])
usb_stop(); return 0; }
if (strncmp(argv[1], "regress", 6) == 0) {if (do_usb_stop_keyboard(0) != 0)return 1;printf("regressing USB..\n");do_usb_regress(argc, argv);return 0;} if (!usb_started) { printf("USB is stopped. Please issue 'usb start' first.\n"); return 1;@@ -821,7 +990,7 @@ static int do_usb(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) }
U_BOOT_CMD(
usb, 5, 1, do_usb,
usb, 6, 1, do_usb, "USB sub-system", "start - start (scan) USB controller\n" "usb reset - reset (rescan) USB controller\n"@@ -831,6 +1000,9 @@ U_BOOT_CMD( "usb test [dev] [port] [mode] - set USB 2.0 test mode\n" " (specify port 0 to indicate the device's upstream port)\n" " Available modes: J, K, S[E0_NAK], P[acket], F[orce_Enable]\n"
"usb regress waddr raddr size [iterations] - regress a USB device\n"" (starts, writes to waddr, reads from raddr, stops and verifies.\n"" `size' format 1B/1K/1M/1G)\n "#ifdef CONFIG_USB_STORAGE "usb storage - show details of USB storage devices\n" "usb dev [dev] - show or set current USB storage device\n" -- 1.9.1
Best Regards, Rajesh Bhagat