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