[RFT PATCH v1 4/5] usb: Provide code to handle spinup of USB usb devices (mostly HDDs)

[Marek Vasut]

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.