usb: Add new command to regress USB devices

PATCH New Rajat Srivastava 6 participants 11 messages Mar 9, 2016 – Mar 13, 2016

Share & tools

Rajat Srivastava

author

2016-03-09 11:22 UTC #0 lore raw

PATCH 1/1 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 <[email protected]>

Signed-off-by Rajesh Bhagat <[email protected]>

---
 common/cmd_usb.c | 174 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 173 insertions(+), 1 deletion(-)

common/cmd_usb.c +173 -1

index a540b42..25fdeab 100644
@@ -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"

Signature · 1 line

1.9.1

Marek Vasut

2016-03-09 11:28 UTC #1 lore raw

PATCH 1/1 usb: Add new command to regress USB devices

Quoted 23 lines · Rajat Srivastava

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

Quoted 2 lines

Signed-off-by Rajat Srivastava <[email protected]>
Signed-off-by Rajesh Bhagat <[email protected]>

Does it do anything which cannot be achieved on the command line itself using "usb reset" "usb write" "usb read" "cmp" commands ?

Signature · 2 lines

Best regards,
Marek Vasut

Hans de Goede

2016-03-09 12:21 UTC #2 lore raw

PATCH 1/1 usb: Add new command to regress USB devices

Hi,

Quoted 24 lines · Marek Vasut

Quoted 23 lines · Rajat Srivastava
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

Quoted 6 lines

Quoted 2 lines
Signed-off-by Rajat Srivastava <[email protected]>
Signed-off-by Rajesh Bhagat <[email protected]>
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

Marek Vasut

2016-03-09 12:55 UTC #3 lore raw

PATCH 1/1 usb: Add new command to regress USB devices

Quoted 27 lines · Hans de Goede

Hi,
Quoted 24 lines · Marek Vasut
Quoted 23 lines · Rajat Srivastava
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

Quoted 8 lines

Quoted 6 lines
Quoted 2 lines
Signed-off-by Rajat Srivastava <[email protected]>
Signed-off-by Rajesh Bhagat <[email protected]>
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 :-)

Quoted 5 lines

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

Quoted 3 lines

Regards,
Hans

Signature · 2 lines

Best regards,
Marek Vasut

Simon Glass

2016-03-09 17:46 UTC #4 lore raw

PATCH 1/1 usb: Add new command to regress USB devices

Hi,

Quoted 41 lines · Marek Vasut

Quoted 27 lines · Hans de Goede
Hi,
Quoted 24 lines · Marek Vasut
Quoted 23 lines · Rajat Srivastava
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
Quoted 8 lines
Quoted 6 lines
Quoted 2 lines
Signed-off-by Rajat Srivastava <[email protected]>
Signed-off-by Rajesh Bhagat <[email protected]>
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 :-)

Quoted 11 lines

Quoted 5 lines
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
Quoted 3 lines
Regards,
Hans

We do have an 'sf test' command. I think it is useful, particularly if it measures speed as well.

Regards,
Simon

Wolfgang Denk

2016-03-13 21:04 UTC #5 lore raw

PATCH 1/1 usb: Add new command to regress USB devices

Dear Simon,

Quoted 3 lines · In message <CAPnjgZ2dAUaRODqssQhpv=2QWSSkQwLtXad0__EnbkuinbeZMg@mail.gmail.com>…

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

Signature · 6 lines

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"The whole problem with the world is  that  fools  and  fanatics  are
always so certain of themselves, but wiser people so full of doubts."
- Bertrand Russell

Rajesh Bhagat

2016-03-09 12:33 UTC #6 lore raw

PATCH 1/1 usb: Add new command to regress USB devices

In reply to Marek Vasut

Quoted 33 lines

-----Original Message-----
From: Marek Vasut [mailto:marex at denx.de]
Sent: Wednesday, March 09, 2016 4:59 PM
To: Rajat Srivastava <[email protected]>; u-boot at lists.denx.de
Cc: sjg at chromium.org; Rajesh Bhagat <[email protected]>
Subject: Re: [PATCH] usb: Add new command to regress USB devices
Quoted 23 lines · Rajat Srivastava
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

Quoted 7 lines

Quoted 2 lines
Signed-off-by Rajat Srivastava <[email protected]>
Signed-off-by Rajesh Bhagat <[email protected]>
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.

Quoted 3 lines

--
Best regards,
Marek Vasut

Marek Vasut

2016-03-09 12:53 UTC #7 lore raw

PATCH 1/1 usb: Add new command to regress USB devices

Quoted 53 lines · Rajesh Bhagat

Quoted 33 lines
-----Original Message-----
From: Marek Vasut [mailto:marex at denx.de]
Sent: Wednesday, March 09, 2016 4:59 PM
To: Rajat Srivastava <[email protected]>; u-boot at lists.denx.de
Cc: sjg at chromium.org; Rajesh Bhagat <[email protected]>
Subject: Re: [PATCH] usb: Add new command to regress USB devices
Quoted 23 lines · Rajat Srivastava
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
Quoted 7 lines
Quoted 2 lines
Signed-off-by Rajat Srivastava <[email protected]>
Signed-off-by Rajesh Bhagat <[email protected]>
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.
common/cmd_usb.c +3 -0
+	"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:

Quoted 8 lines

    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

Signature · 2 lines

Best regards,
Marek Vasut

Simon Glass

2016-03-09 21:39 UTC #8 lore raw

PATCH 1/1 usb: Add new command to regress USB devices

In reply to Rajat Srivastava

Hi Rajat,

Quoted 23 lines · Rajat Srivastava

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

Quoted 5 lines

Signed-off-by Rajat Srivastava <[email protected]>
Signed-off-by Rajesh Bhagat <[email protected]>
---
 common/cmd_usb.c | 174 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 173 insertions(+), 1 deletion(-)

Can you rebase to mainline? This file has been renamed.

Quoted 14 lines

common/cmd_usb.c +1 -0

index a540b42..25fdeab 100644
@@ -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 */

common/cmd_usb.c static int usb_device_info(void) · line 620

Quoted 4 lines

common/cmd_usb.c +1 -0

@@ -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

Quoted 44 lines

common/cmd_usb.c +44 -0

+{
+       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] };
+

common/cmd_usb.c static int usb_device_info(void) · line 674

Quoted 10 lines

common/cmd_usb.c +10 -0

+       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.

common/cmd_usb.c static int usb_device_info(void) · line 679

Quoted 5 lines

common/cmd_usb.c +5 -0

+       return 0;
+}
+
+
+static int do_usb_regress(int argc, char * const argv[])

Would 'usb datatest' be a better name?

Quoted 42 lines

common/cmd_usb.c +42 -0

+{
+       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) {

common/cmd_usb.c static int usb_device_info(void) · line 731

Quoted 10 lines

common/cmd_usb.c +10 -0

+                       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?

common/cmd_usb.c static int usb_device_info(void) · line 733

Quoted 2 lines

common/cmd_usb.c +2 -0

+               /* 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.

Quoted 15 lines

common/cmd_usb.c +15 -0

+               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);

common/cmd_usb.c static int usb_device_info(void) · line 758

Quoted 10 lines

common/cmd_usb.c +10 -0

+               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.

common/cmd_usb.c static int usb_device_info(void) · line 760

Quoted 2 lines

common/cmd_usb.c +2 -0
+               }
+#endif

Can this test pass on sandbox?

common/cmd_usb.c static int usb_device_info(void) · line 763

Quoted 3 lines

common/cmd_usb.c +3 -0

+
+               /* stop USB */
+               printf("stopping USB..\n");

It would be good to display the average read and write transfer speed here. See 'sf test'.

Quoted 56 lines

common/cmd_usb.c +28 -2

+               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

Rajesh Bhagat

2016-03-10 04:52 UTC #9 lore raw

PATCH 1/1 usb: Add new command to regress USB devices

Quoted 41 lines

-----Original Message-----
From: sjg at google.com [mailto:sjg at google.com] On Behalf Of Simon Glass
Sent: Thursday, March 10, 2016 3:09 AM
To: Rajat Srivastava <[email protected]>
Cc: U-Boot Mailing List <[email protected]>; Marek Va?ut <[email protected]>; Rajesh Bhagat <[email protected]>
Subject: Re: [PATCH] usb: Add new command to regress USB devices
Hi Rajat,
Quoted 23 lines · Rajat Srivastava
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
Quoted 5 lines
Signed-off-by Rajat Srivastava <[email protected]>
Signed-off-by Rajesh Bhagat <[email protected]>
---
 common/cmd_usb.c | 174

Quoted 6 lines

++++++++++++++++++++++++++++++++++++++++++++++++++++
++-
Quoted 1 line
 1 file changed, 173 insertions(+), 1 deletion(-)
Can you rebase to mainline? This file has been renamed.

Will take care v2.

Quoted 15 lines

common/cmd_usb.c index renamed from common/cmd_usb.c +0 -0

a540b42..25fdeab 100644

common/cmd_usb.c +2 -0

@@ -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

Quoted 8 lines

Quoted 1 line
+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.

Quoted 57 lines

Quoted 44 lines

common/cmd_usb.c +44 -0

+{
+       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] };
+

common/cmd_usb.c static int usb_device_info(void) · line 674

Quoted 10 lines

common/cmd_usb.c +10 -0

+       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.

Quoted 3 lines

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.

Quoted 8 lines

common/cmd_usb.c static int usb_device_info(void) · line 679
Quoted 5 lines
common/cmd_usb.c +5 -0
+       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.

Quoted 47 lines

common/cmd_usb.c +47 -0

+{
+       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) {

Quoted 9 lines

common/cmd_usb.c static int usb_device_info(void) · line 731
Quoted 6 lines
common/cmd_usb.c +6 -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.

Quoted 5 lines

common/cmd_usb.c static int usb_device_info(void) · line 733
Quoted 2 lines
common/cmd_usb.c +2 -0
+               /* 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

Quoted 26 lines

Quoted 15 lines

common/cmd_usb.c +15 -0

+               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);

common/cmd_usb.c static int usb_device_info(void) · line 758

Quoted 10 lines

common/cmd_usb.c +10 -0

+               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;

Quoted 2 lines

I think this needs to be CMD_RET_FAILURE.

Agreed. Will take care v2

Quoted 5 lines

common/cmd_usb.c
Quoted 2 lines
common/cmd_usb.c +2 -0
+               }
+#endif
Can this test pass on sandbox?

Ok, We try to make its setup and share results.

Quoted 7 lines

common/cmd_usb.c static int usb_device_info(void) · line 763
Quoted 3 lines
common/cmd_usb.c +3 -0
+
+               /* 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.

Quoted 51 lines

Quoted 8 lines

common/cmd_usb.c +7 -0

+               usb_stop();
+
+#ifdef CONFIG_USB_STORAGE
+               /*
+                * verify the content written on USB and
+                * content read from USB.
+                */

common/cmd_usb.c static int usb_device_info(void) · line 780

Quoted 10 lines

common/cmd_usb.c +10 -0

+               if (usb_read_write_verify(w_addr, r_addr, cnt) == -1)
+                       return -1;
+#endif
+               if (ctrlc())
+                       return -1;
+       }
+
+       return 0;
+}
+

/**********************************************************************
********

common/cmd_usb.c static int do_usb(cmd_tbl_t *cmdtp, int flag, int argc, char

Quoted 3 lines

* usb command intepreter
*/
common/cmd_usb.c +0 -0
@@ -656,6 +818,13 @@ static int do_usb(cmd_tbl_t *cmdtp, int flag, int argc, char

* const argv[])

Quoted 27 lines

common/cmd_usb.c +9 -1

                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"

Quoted 8 lines

common/cmd_usb.c +2 -1

+       "    (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

Simon Glass

2016-03-13 02:52 UTC #10 lore raw

PATCH 1/1 usb: Add new command to regress USB devices

Hi,

Quoted 133 lines · Rajesh Bhagat

Quoted 41 lines
-----Original Message-----
From: sjg at google.com [mailto:sjg at google.com] On Behalf Of Simon Glass
Sent: Thursday, March 10, 2016 3:09 AM
To: Rajat Srivastava <[email protected]>
Cc: U-Boot Mailing List <[email protected]>; Marek Va?ut <[email protected]>; Rajesh Bhagat <[email protected]>
Subject: Re: [PATCH] usb: Add new command to regress USB devices
Hi Rajat,
Quoted 23 lines · Rajat Srivastava
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
Quoted 5 lines
Signed-off-by Rajat Srivastava <[email protected]>
Signed-off-by Rajesh Bhagat <[email protected]>
---
 common/cmd_usb.c | 174
Quoted 6 lines
++++++++++++++++++++++++++++++++++++++++++++++++++++
++-
Quoted 1 line
 1 file changed, 173 insertions(+), 1 deletion(-)
Can you rebase to mainline? This file has been renamed.
Will take care v2.
Quoted 15 lines
Quoted 15 lines
common/cmd_usb.c index renamed from common/cmd_usb.c +0 -0
a540b42..25fdeab 100644
common/cmd_usb.c +2 -0
@@ -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
Quoted 8 lines
Quoted 1 line
+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.
Quoted 57 lines
Quoted 44 lines
common/cmd_usb.c +44 -0
+{
+       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] };
+
common/cmd_usb.c static int usb_device_info(void) · line 674
Quoted 10 lines
common/cmd_usb.c +10 -0
+       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.

Quoted 6 lines

Quoted 3 lines
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.

Quoted 10 lines

Quoted 9 lines
common/cmd_usb.c static int usb_device_info(void) · line 679
Quoted 6 lines
common/cmd_usb.c +5 -0
+       return 0;
+}
+
+
+static int do_usb_regress(int argc, char * const argv[])
Would 'usb datatest' be a better name?

Quoted 7 lines

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