| Message ID | 20200322130031.10455-5-lukma@denx.de |
|---|---|
| State | New |
| Headers | show |
| Series | usb: Improve robustness of ehci-hcd controller operation | expand |
On 3/22/20 2:00 PM, Lukasz Majewski wrote: [...] > diff --git a/common/usb_storage.c b/common/usb_storage.c > index 92e1e54d1c..3c2324fa1a 100644 > --- a/common/usb_storage.c > +++ b/common/usb_storage.c > @@ -729,6 +729,7 @@ static int usb_stor_BBB_transport(struct scsi_cmd *srb, struct us_data *us) > pipeout = usb_sndbulkpipe(us->pusb_dev, us->ep_out); > /* DATA phase + error handling */ > data_actlen = 0; > + mdelay(10); /* Like linux does. */ Does this add delay to every single transfer ? That would mean a massive slowdown if you use short data transfers, which is needed for old/limited USB sticks. > /* no data, go immediately to the STATUS phase */ > if (srb->datalen == 0) > goto st; > @@ -1023,9 +1024,32 @@ static int usb_request_sense(struct scsi_cmd *srb, struct us_data *ss) > return 0; > } > > +/* > + * This spins up the disk and also consumes the time that the > + * disk takes to become active and ready to read data. > + * Some drives (like Western Digital) can take more than 5 seconds. > + * The delay occurs on the 1st data read from the disk. > + * Extending the timeout here works better than handling the timeout > + * as an error on a "real" read. > + */ > +static int usb_spinup(struct scsi_cmd *srb, struct us_data *ss) > +{ > + memset(&srb->cmd[0], 0, 12); > + srb->cmd[0] = SCSI_START_STP; > + srb->cmd[1] = srb->lun << 5; > + srb->cmd[4] = 1; /* Start spinup. */ > + srb->datalen = 0; > + srb->cmdlen = 6; > + ss->pusb_dev->extra_timeout = 9876; What is this magic number ? > + ss->transport(srb, ss); > + ss->pusb_dev->extra_timeout = 0; > + return 0; > +} [...] > diff --git a/include/usb.h b/include/usb.h > index efb67ea33f..5b0f5ce5d6 100644 > --- a/include/usb.h > +++ b/include/usb.h > @@ -140,6 +140,7 @@ struct usb_device { > int act_len; /* transferred bytes */ > int maxchild; /* Number of ports if hub */ > int portnr; /* Port number, 1=first */ > + int extra_timeout; /* Add to timeout in ehci_submit_async or spinup */ Does this work with xhci too ?
Hi Marek, > On 3/22/20 2:00 PM, Lukasz Majewski wrote: > [...] > > diff --git a/common/usb_storage.c b/common/usb_storage.c > > index 92e1e54d1c..3c2324fa1a 100644 > > --- a/common/usb_storage.c > > +++ b/common/usb_storage.c > > @@ -729,6 +729,7 @@ static int usb_stor_BBB_transport(struct > > scsi_cmd *srb, struct us_data *us) pipeout = > > usb_sndbulkpipe(us->pusb_dev, us->ep_out); /* DATA phase + error > > handling */ data_actlen = 0; > > + mdelay(10); /* Like linux does. */ > > Does this add delay to every single transfer ? It brings the slowdown, yes. However, without it I very often see the error that the USB address couldn't be assigned. > That would mean a massive slowdown if you use short data transfers, > which is needed for old/limited USB sticks. > > > /* no data, go immediately to the STATUS phase */ > > if (srb->datalen == 0) > > goto st; > > @@ -1023,9 +1024,32 @@ static int usb_request_sense(struct scsi_cmd > > *srb, struct us_data *ss) return 0; > > } > > > > +/* > > + * This spins up the disk and also consumes the time that the > > + * disk takes to become active and ready to read data. > > + * Some drives (like Western Digital) can take more than 5 seconds. > > + * The delay occurs on the 1st data read from the disk. > > + * Extending the timeout here works better than handling the > > timeout > > + * as an error on a "real" read. > > + */ > > +static int usb_spinup(struct scsi_cmd *srb, struct us_data *ss) > > +{ > > + memset(&srb->cmd[0], 0, 12); > > + srb->cmd[0] = SCSI_START_STP; > > + srb->cmd[1] = srb->lun << 5; > > + srb->cmd[4] = 1; /* Start spinup. */ > > + srb->datalen = 0; > > + srb->cmdlen = 6; > > + ss->pusb_dev->extra_timeout = 9876; > > What is this magic number ? This number is added to the timeout up to which ehci controller waits for EHCI TD to be sent. Why there is 9876 - I do guess that it was took from Linux in some point in time. > > > + ss->transport(srb, ss); > > + ss->pusb_dev->extra_timeout = 0; > > + return 0; > > +} > > [...] > > > diff --git a/include/usb.h b/include/usb.h > > index efb67ea33f..5b0f5ce5d6 100644 > > --- a/include/usb.h > > +++ b/include/usb.h > > @@ -140,6 +140,7 @@ struct usb_device { > > int act_len; /* transferred bytes */ > > int maxchild; /* Number of ports if > > hub */ int portnr; /* Port number, 1=first */ > > + int extra_timeout; /* Add to timeout in ehci_submit_async > > or spinup */ > > Does this work with xhci too ? Yes, it is used in patch 5/5. Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 488 bytes Desc: OpenPGP digital signature URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200323/d7fd11d3/attachment.sig>
On 3/23/20 8:53 AM, Lukasz Majewski wrote: > Hi Marek, Hi, >> On 3/22/20 2:00 PM, Lukasz Majewski wrote: >> [...] >>> diff --git a/common/usb_storage.c b/common/usb_storage.c >>> index 92e1e54d1c..3c2324fa1a 100644 >>> --- a/common/usb_storage.c >>> +++ b/common/usb_storage.c >>> @@ -729,6 +729,7 @@ static int usb_stor_BBB_transport(struct >>> scsi_cmd *srb, struct us_data *us) pipeout = >>> usb_sndbulkpipe(us->pusb_dev, us->ep_out); /* DATA phase + error >>> handling */ data_actlen = 0; >>> + mdelay(10); /* Like linux does. */ >> >> Does this add delay to every single transfer ? > > It brings the slowdown, yes. > > However, without it I very often see the error that the USB address > couldn't be assigned. Seems like this is hiding some real error then. If I do basic math, then I reach a conclusion that the comment is bogus. Look, assume the transfer itself takes 0 time, then if you have 10 mS delays between transfers, you can do 100 transfer per second. If one transfer is 240 blocks * 512 bytes , then you are limited to 12.2 MB/s. And I am positive USB 2.0 sticks in Linux can transfer faster than that. >> That would mean a massive slowdown if you use short data transfers, >> which is needed for old/limited USB sticks. >> >>> /* no data, go immediately to the STATUS phase */ >>> if (srb->datalen == 0) >>> goto st; >>> @@ -1023,9 +1024,32 @@ static int usb_request_sense(struct scsi_cmd >>> *srb, struct us_data *ss) return 0; >>> } >>> >>> +/* >>> + * This spins up the disk and also consumes the time that the >>> + * disk takes to become active and ready to read data. >>> + * Some drives (like Western Digital) can take more than 5 seconds. >>> + * The delay occurs on the 1st data read from the disk. >>> + * Extending the timeout here works better than handling the >>> timeout >>> + * as an error on a "real" read. >>> + */ >>> +static int usb_spinup(struct scsi_cmd *srb, struct us_data *ss) >>> +{ >>> + memset(&srb->cmd[0], 0, 12); >>> + srb->cmd[0] = SCSI_START_STP; >>> + srb->cmd[1] = srb->lun << 5; >>> + srb->cmd[4] = 1; /* Start spinup. */ >>> + srb->datalen = 0; >>> + srb->cmdlen = 6; >>> + ss->pusb_dev->extra_timeout = 9876; >> >> What is this magic number ? > > This number is added to the timeout up to which ehci controller waits > for EHCI TD to be sent. This is generic code and has to work with OHCI/UHCI/xHCI too. > Why there is 9876 - I do guess that it was took from Linux in some > point in time. Please, research it. >>> + ss->transport(srb, ss); >>> + ss->pusb_dev->extra_timeout = 0; >>> + return 0; >>> +} >> >> [...] >> >>> diff --git a/include/usb.h b/include/usb.h >>> index efb67ea33f..5b0f5ce5d6 100644 >>> --- a/include/usb.h >>> +++ b/include/usb.h >>> @@ -140,6 +140,7 @@ struct usb_device { >>> int act_len; /* transferred bytes */ >>> int maxchild; /* Number of ports if >>> hub */ int portnr; /* Port number, 1=first */ >>> + int extra_timeout; /* Add to timeout in ehci_submit_async >>> or spinup */ >> >> Does this work with xhci too ? > > Yes, it is used in patch 5/5. Does xhci need it ?
Hi Marek, > On 3/23/20 8:53 AM, Lukasz Majewski wrote: > > Hi Marek, > > Hi, > > >> On 3/22/20 2:00 PM, Lukasz Majewski wrote: > >> [...] > >>> diff --git a/common/usb_storage.c b/common/usb_storage.c > >>> index 92e1e54d1c..3c2324fa1a 100644 > >>> --- a/common/usb_storage.c > >>> +++ b/common/usb_storage.c > >>> @@ -729,6 +729,7 @@ static int usb_stor_BBB_transport(struct > >>> scsi_cmd *srb, struct us_data *us) pipeout = > >>> usb_sndbulkpipe(us->pusb_dev, us->ep_out); /* DATA phase + error > >>> handling */ data_actlen = 0; > >>> + mdelay(10); /* Like linux does. */ > >> > >> Does this add delay to every single transfer ? > > > > It brings the slowdown, yes. > > > > However, without it I very often see the error that the USB address > > couldn't be assigned. > > Seems like this is hiding some real error then. > > If I do basic math, then I reach a conclusion that the comment is > bogus. Look, assume the transfer itself takes 0 time, then if you > have 10 mS delays between transfers, you can do 100 transfer per > second. If one transfer is 240 blocks * 512 bytes , then you are > limited to 12.2 MB/s. And I am positive USB 2.0 sticks in Linux can > transfer faster than that. Theoretically USB 2.0 can have up to 60MiB/s transfer rate. The 12MiB/s is for USB 1.x. I cannot say why this delay helps with recognition of some devices. I also start wondering why I do see some strange problems in U-Boot (like not assigning address, timeouts), as those errors are not present on Linux. > > >> That would mean a massive slowdown if you use short data transfers, > >> which is needed for old/limited USB sticks. > >> > >>> /* no data, go immediately to the STATUS phase */ > >>> if (srb->datalen == 0) > >>> goto st; > >>> @@ -1023,9 +1024,32 @@ static int usb_request_sense(struct > >>> scsi_cmd *srb, struct us_data *ss) return 0; > >>> } > >>> > >>> +/* > >>> + * This spins up the disk and also consumes the time that the > >>> + * disk takes to become active and ready to read data. > >>> + * Some drives (like Western Digital) can take more than 5 > >>> seconds. > >>> + * The delay occurs on the 1st data read from the disk. > >>> + * Extending the timeout here works better than handling the > >>> timeout > >>> + * as an error on a "real" read. > >>> + */ > >>> +static int usb_spinup(struct scsi_cmd *srb, struct us_data *ss) > >>> +{ > >>> + memset(&srb->cmd[0], 0, 12); > >>> + srb->cmd[0] = SCSI_START_STP; > >>> + srb->cmd[1] = srb->lun << 5; > >>> + srb->cmd[4] = 1; /* Start spinup. */ > >>> + srb->datalen = 0; > >>> + srb->cmdlen = 6; > >>> + ss->pusb_dev->extra_timeout = 9876; > >> > >> What is this magic number ? > > > > This number is added to the timeout up to which ehci controller > > waits for EHCI TD to be sent. > > This is generic code and has to work with OHCI/UHCI/xHCI too. > > > Why there is 9876 - I do guess that it was took from Linux in some > > point in time. > > Please, research it. > > >>> + ss->transport(srb, ss); > >>> + ss->pusb_dev->extra_timeout = 0; > >>> + return 0; > >>> +} > >> > >> [...] > >> > >>> diff --git a/include/usb.h b/include/usb.h > >>> index efb67ea33f..5b0f5ce5d6 100644 > >>> --- a/include/usb.h > >>> +++ b/include/usb.h > >>> @@ -140,6 +140,7 @@ struct usb_device { > >>> int act_len; /* transferred bytes > >>> */ int maxchild; /* Number of ports if > >>> hub */ int portnr; /* Port number, 1=first > >>> */ > >>> + int extra_timeout; /* Add to timeout in ehci_submit_async > >>> or spinup */ > >> > >> Does this work with xhci too ? > > > > Yes, it is used in patch 5/5. > > Does xhci need it ? > It is hard to say. The original fix was for EHCI. Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 488 bytes Desc: OpenPGP digital signature URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200323/e0c86ad7/attachment.sig>
On 3/23/20 1:54 PM, Lukasz Majewski wrote: > Hi Marek, Hi, >> On 3/23/20 8:53 AM, Lukasz Majewski wrote: >>> Hi Marek, >> >> Hi, >> >>>> On 3/22/20 2:00 PM, Lukasz Majewski wrote: >>>> [...] >>>>> diff --git a/common/usb_storage.c b/common/usb_storage.c >>>>> index 92e1e54d1c..3c2324fa1a 100644 >>>>> --- a/common/usb_storage.c >>>>> +++ b/common/usb_storage.c >>>>> @@ -729,6 +729,7 @@ static int usb_stor_BBB_transport(struct >>>>> scsi_cmd *srb, struct us_data *us) pipeout = >>>>> usb_sndbulkpipe(us->pusb_dev, us->ep_out); /* DATA phase + error >>>>> handling */ data_actlen = 0; >>>>> + mdelay(10); /* Like linux does. */ >>>> >>>> Does this add delay to every single transfer ? >>> >>> It brings the slowdown, yes. >>> >>> However, without it I very often see the error that the USB address >>> couldn't be assigned. >> >> Seems like this is hiding some real error then. >> >> If I do basic math, then I reach a conclusion that the comment is >> bogus. Look, assume the transfer itself takes 0 time, then if you >> have 10 mS delays between transfers, you can do 100 transfer per >> second. If one transfer is 240 blocks * 512 bytes , then you are >> limited to 12.2 MB/s. And I am positive USB 2.0 sticks in Linux can >> transfer faster than that. > > Theoretically USB 2.0 can have up to 60MiB/s transfer rate. The 12MiB/s > is for USB 1.x. If you add such a massive 10 mS delay between each and every single read, then the performance will be much lower, see the simple calculation above. > I cannot say why this delay helps with recognition of some devices. I > also start wondering why I do see some strange problems in U-Boot (like > not assigning address, timeouts), as those errors are not present on > Linux. Maybe something to investigate in more depth ? [...] >>>>> diff --git a/include/usb.h b/include/usb.h >>>>> index efb67ea33f..5b0f5ce5d6 100644 >>>>> --- a/include/usb.h >>>>> +++ b/include/usb.h >>>>> @@ -140,6 +140,7 @@ struct usb_device { >>>>> int act_len; /* transferred bytes >>>>> */ int maxchild; /* Number of ports if >>>>> hub */ int portnr; /* Port number, 1=first >>>>> */ >>>>> + int extra_timeout; /* Add to timeout in ehci_submit_async >>>>> or spinup */ >>>> >>>> Does this work with xhci too ? >>> >>> Yes, it is used in patch 5/5. >> >> Does xhci need it ? >> > > It is hard to say. The original fix was for EHCI. Please check. This needs to be fixed properly instead of just papering over the problem and hacking it up somehow to make it somehow work.
diff --git a/common/usb_storage.c b/common/usb_storage.c index 92e1e54d1c..3c2324fa1a 100644 --- a/common/usb_storage.c +++ b/common/usb_storage.c @@ -729,6 +729,7 @@ static int usb_stor_BBB_transport(struct scsi_cmd *srb, struct us_data *us) pipeout = usb_sndbulkpipe(us->pusb_dev, us->ep_out); /* DATA phase + error handling */ data_actlen = 0; + mdelay(10); /* Like linux does. */ /* no data, go immediately to the STATUS phase */ if (srb->datalen == 0) goto st; @@ -1023,9 +1024,32 @@ static int usb_request_sense(struct scsi_cmd *srb, struct us_data *ss) return 0; } +/* + * This spins up the disk and also consumes the time that the + * disk takes to become active and ready to read data. + * Some drives (like Western Digital) can take more than 5 seconds. + * The delay occurs on the 1st data read from the disk. + * Extending the timeout here works better than handling the timeout + * as an error on a "real" read. + */ +static int usb_spinup(struct scsi_cmd *srb, struct us_data *ss) +{ + memset(&srb->cmd[0], 0, 12); + srb->cmd[0] = SCSI_START_STP; + srb->cmd[1] = srb->lun << 5; + srb->cmd[4] = 1; /* Start spinup. */ + srb->datalen = 0; + srb->cmdlen = 6; + ss->pusb_dev->extra_timeout = 9876; + ss->transport(srb, ss); + ss->pusb_dev->extra_timeout = 0; + return 0; +} + static int usb_test_unit_ready(struct scsi_cmd *srb, struct us_data *ss) { int retries = 10; + int gave_extra_time = 0; do { memset(&srb->cmd[0], 0, 12); @@ -1048,6 +1072,17 @@ static int usb_test_unit_ready(struct scsi_cmd *srb, struct us_data *ss) if ((srb->sense_buf[2] == 0x02) && (srb->sense_buf[12] == 0x3a)) return -1; + /* + * If the status is "Not Ready - becoming ready", give it + * more time. Linux issues a spinup command (once) and gives + * it 100 seconds. + */ + if (srb->sense_buf[2] == 0x02 && + srb->sense_buf[12] == 0x04 && + gave_extra_time == 0) { + retries = 100; /* Allow 10 seconds. */ + gave_extra_time = retries; + } mdelay(100); } while (retries--); @@ -1502,6 +1537,7 @@ int usb_stor_get_info(struct usb_device *dev, struct us_data *ss, dev_desc->log2blksz = LOG2(dev_desc->blksz); dev_desc->type = perq; debug(" address %d\n", dev_desc->target); + usb_spinup(pccb, ss); return 1; } diff --git a/include/usb.h b/include/usb.h index efb67ea33f..5b0f5ce5d6 100644 --- a/include/usb.h +++ b/include/usb.h @@ -140,6 +140,7 @@ struct usb_device { int act_len; /* transferred bytes */ int maxchild; /* Number of ports if hub */ int portnr; /* Port number, 1=first */ + int extra_timeout; /* Add to timeout in ehci_submit_async or spinup */ #if !CONFIG_IS_ENABLED(DM_USB) /* parent hub, or NULL if this is the root hub */ struct usb_device *parent;
After this change USB mass storage devices - mostly HDDs connected via USB - will gain handling of extra time needed for their spin up. This operation is realized with issuing SCSI start/stop unit (0x1B) command. This code is the port to newest U-Boot of the fix from - "rayvt" (from [1]). Links: [1] - https://forum.doozan.com/read.php?3,35295,35295#msg-35295 [2] - https://www.dropbox.com/s/nrkrd1no63viuu8/uboot-bodhi-2016.05-timeoutTD.patch?dl=0 Signed-off-by: Lukasz Majewski <lukma at denx.de> [Unfortunately, the original patch [2] did not contain S-o-B from the original author - "rayvt"] --- common/usb_storage.c | 36 ++++++++++++++++++++++++++++++++++++ include/usb.h | 1 + 2 files changed, 37 insertions(+)