[U-Boot] [PATCH] usb: Add new command to regress USB devices

[Simon Glass]

Hi,

On 9 March 2016 at 21:52, Rajesh Bhagat <rajesh.bhagat at nxp.com> wrote:
>
>
>> -----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 <rajat.srivastava at nxp.com>
>> Cc: U-Boot Mailing List <u-boot at lists.denx.de>; Marek Vašut <marex at denx.de>;
>> Rajesh Bhagat <rajesh.bhagat at 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 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.
>>
>
> 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