| Message ID | 1457522546-12840-1-git-send-email-rajat.srivastava@nxp.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Simon Glass |
| Headers | show |
> -----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: > > > > 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> > > > 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
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: > >>> > >>> 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> > >> > >> > >> 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
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: > > 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(-) 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 >
> -----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: > > > > 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(-) > > 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; > > + 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? > 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: >> > >> > 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(-) >> >> 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
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
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"
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(-)