> -----Original Message-----
> From: s...@google.com [mailto:s...@google.com] On Behalf Of Simon Glass
> Sent: Thursday, March 10, 2016 3:09 AM
> To: Rajat Srivastava <rajat.srivast...@nxp.com>
> Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Marek VaĊĦut <ma...@denx.de>;
> Rajesh Bhagat <rajesh.bha...@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.srivast...@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.srivast...@nxp.com>
> > Signed-off-by: Rajesh Bhagat <rajesh.bha...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
