[U-Boot] [PATCH] usb: Add new command to regress USB devices
This patch adds a new 'usb regress' command, that can be used to regress test a USB device. It performs the following operations:
1. starts the USB device 2. performs read/write operations 3. stops the USB device 4. 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(-)
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) +{ + 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); + return 0; +} + + +static int do_usb_regress(int argc, char * const argv[]) +{ + 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; + else + loopcount = 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 + /* write on USB from address (w_addr) of RAM */ + w_addr = simple_strtoul(argv[2], NULL, 16); + 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; + } +#endif + + /* stop USB */ + printf("stopping USB..\n"); + 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"
On 03/09/2016 12:22 PM, Rajat Srivastava 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
Does it do anything which cannot be achieved on the command line itself using "usb reset" "usb write" "usb read" "cmp" commands ?
Hi,
On 09-03-16 12:28, Marek Vasut wrote:
On 03/09/2016 12:22 PM, Rajat Srivastava 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
Does it do anything which cannot be achieved on the command line itself using "usb reset" "usb write" "usb read" "cmp" commands ?
This seems to be about a reading / writing a usb-disk / usb-storage device. I believe this can certainly be achieved with the existing disk io commands, and moreover this seems quite dangerous (overwriting the partition table on the device), so I think requiring the user to do this explicitly indeed seems better.
Regards,
Hans
On 03/09/2016 01:21 PM, Hans de Goede wrote:
Hi,
On 09-03-16 12:28, Marek Vasut wrote:
On 03/09/2016 12:22 PM, Rajat Srivastava 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
Does it do anything which cannot be achieved on the command line itself using "usb reset" "usb write" "usb read" "cmp" commands ?
This seems to be about a reading / writing a usb-disk / usb-storage device.
That's what usb read / usb write commands are for, reading raw data from USB disk :-)
I believe this can certainly be achieved with the existing disk io commands, and moreover this seems quite dangerous (overwriting the partition table on the device), so I think requiring the user to do this explicitly indeed seems better.
Yeah
Regards,
Hans
Hi,
On 9 March 2016 at 05:55, Marek Vasut marex@denx.de wrote:
On 03/09/2016 01:21 PM, Hans de Goede wrote:
Hi,
On 09-03-16 12:28, Marek Vasut wrote:
On 03/09/2016 12:22 PM, Rajat Srivastava 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
Does it do anything which cannot be achieved on the command line itself using "usb reset" "usb write" "usb read" "cmp" commands ?
This seems to be about a reading / writing a usb-disk / usb-storage device.
That's what usb read / usb write commands are for, reading raw data from USB disk :-)
I believe this can certainly be achieved with the existing disk io commands, and moreover this seems quite dangerous (overwriting the partition table on the device), so I think requiring the user to do this explicitly indeed seems better.
Yeah
Regards,
Hans
We do have an 'sf test' command. I think it is useful, particularly if it measures speed as well.
Regards, Simon
Dear Simon,
In message CAPnjgZ2dAUaRODqssQhpv=2QWSSkQwLtXad0__EnbkuinbeZMg@mail.gmail.com you wrote:
We do have an 'sf test' command. I think it is useful, particularly if it measures speed as well.
I tend to agree with Hans - this should normally be implemented in a script, or, if reallyneeed (but I can't ssee why this would be necessary) in board specific code.
If added as a generic feature, then there should be options to select which range of the device to use for testing. It may be interesting to test specific areas (like at the assumed end of the device), or at least to be able to respect existing partitions etc.
But still - I can't see why this would be needed - plain reading and writing to the device or even to a file in a file system should perform the same task.
Best regards,
Wolfgang Denk
-----Original Message----- From: Marek Vasut [mailto:marex@denx.de] Sent: Wednesday, March 09, 2016 4:59 PM To: Rajat Srivastava rajat.srivastava@nxp.com; u-boot@lists.denx.de Cc: sjg@chromium.org; Rajesh Bhagat rajesh.bhagat@nxp.com Subject: Re: [PATCH] usb: Add new command to regress USB devices
On 03/09/2016 12:22 PM, Rajat Srivastava 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
Does it do anything which cannot be achieved on the command line itself using "usb reset" "usb write" "usb read" "cmp" commands ?
Let me share little background and motivation for addition of this command:
1. We need to test different make(from different vendors/model) USB devices to test USB hardware. Where we generally face below issues: a. USB devices enumeration failure on Nth iteration. b. USB read/write failure (incomplete transfer or data corruption) for particular data size e.g. 12M. 2. "usb regress" command takes size/iterations and performs all above operations in single command to reduce manual overhead. + "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 " 3. We are planning to provide a patch over it to provide summary report as below: Regress Report: USB enumerate: OK/ERROR (2/20) USB write: OK/ERROR (2/20) USB read: OK/ERROR (2/20) USB verify: OK/ERROR (2/20) Above report can be useful to regress a USB devices, and detailed report need to referred only when anything fails.
Please provide your opinion on the same.
-- Best regards, Marek Vasut
On 03/09/2016 01:33 PM, Rajesh Bhagat wrote:
-----Original Message----- From: Marek Vasut [mailto:marex@denx.de] Sent: Wednesday, March 09, 2016 4:59 PM To: Rajat Srivastava rajat.srivastava@nxp.com; u-boot@lists.denx.de Cc: sjg@chromium.org; Rajesh Bhagat rajesh.bhagat@nxp.com Subject: Re: [PATCH] usb: Add new command to regress USB devices
On 03/09/2016 12:22 PM, Rajat Srivastava 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
Does it do anything which cannot be achieved on the command line itself using "usb reset" "usb write" "usb read" "cmp" commands ?
Let me share little background and motivation for addition of this command:
- We need to test different make(from different vendors/model) USB devices to test USB hardware. Where we
generally face below issues: a. USB devices enumeration failure on Nth iteration. b. USB read/write failure (incomplete transfer or data corruption) for particular data size e.g. 12M. 2. "usb regress" command takes size/iterations and performs all above operations in single command to reduce manual overhead.
- "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 "
- We are planning to provide a patch over it to provide summary report as below: Regress Report: USB enumerate: OK/ERROR (2/20) USB write: OK/ERROR (2/20) USB read: OK/ERROR (2/20) USB verify: OK/ERROR (2/20)
Above report can be useful to regress a USB devices, and detailed report need to referred only when anything fails.
Please provide your opinion on the same.
Did you measure the overhead ? I believe the overhead of implementing this in shell is significantly lower than the overhead of the USB and the USB stack itself, so discussing any overhead concerns here is moot.
Yet still, I cannot find a definitive answer to my question whether this can be implemented by pure u-boot shell commands, but I think it can be done that way. So why should I accept this patch over something like:
while true ; do \ usb reset & usb write .. && usb read ... && cmp ... && echo OK ; \ done
?
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.
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
+{
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.
return 0;+}
+static int do_usb_regress(int argc, char * const argv[])
Would 'usb datatest' be a better name?
+{
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?
/* 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.
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.
}+#endif
Can this test pass on sandbox?
/* stop USB */printf("stopping USB..\n");
It would be good to display the average read and write transfer speed here. See 'sf test'.
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
-----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
Hi,
On 9 March 2016 at 21:52, Rajesh Bhagat rajesh.bhagat@nxp.com wrote:
-----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.
Yes that sounds right.
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.
I like the idea, but I don't think we can rename an existing command without a lot of thought. While I agree with your sentiment, since your command can be destructive, I think it is best not to do this. Existing scripts may start overwriting data on USB sticks.
[snip]
Regards, Simon
participants (6)
-
Hans de Goede -
Marek Vasut -
Rajat Srivastava -
Rajesh Bhagat -
Simon Glass -
Wolfgang Denk