[U-Boot] [PATCH] usb: Add new command to regress USB devices
Simon Glass
sjg at chromium.org
Wed Mar 9 22:39:20 CET 2016
Hi Rajat,
On 9 March 2016 at 04:22, Rajat Srivastava <rajat.srivastava at 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:
>
> 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 at nxp.com>
> Signed-off-by: Rajesh Bhagat <rajesh.bhagat at 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;
> + 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
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
>
More information about the U-Boot
mailing list