Hi, On 9 March 2016 at 21:52, Rajesh Bhagat <[email protected]> wrote: > > >> -----Original Message----- >> From: [email protected] [mailto:[email protected]] 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, >> >> On 9 March 2016 at 04:22, Rajat Srivastava <[email protected]> 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 <[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. >> > > 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 _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
