[U-Boot] [PATCH v2] usb: fix usb_stor_read/write on DM

[Benoît Thébaudeau]

On Fri, Jul 14, 2017 at 1:50 PM, Marek Vasut <marex at denx.de> wrote:
> On 07/14/2017 01:03 PM, Masahiro Yamada wrote:
>> 2017-07-14 19:07 GMT+09:00 Marek Vasut <marex at denx.de>:
>>> On 07/14/2017 04:31 AM, Masahiro Yamada wrote:
>>>> Prior to DM, we could not enable different types of USB controllers
>>>> at the same time.  DM was supposed to loosen the limitation.  It is
>>>> true that we can compile drivers, but they do not work.
>>>>
>>>> For example, if EHCI is enabled, xHCI fails as follows:
>>>>
>>>>   => usb read 82000000 0 2000
>>>>
>>>>   USB read: device 0 block # 0, count 8192 ... WARN halted endpoint, queueing URB anyway.
>>>>   Unexpected XHCI event TRB, skipping... (3fb54010 00000001 13000000 01008401)
>>>>   BUG: failure at drivers/usb/host/xhci-ring.c:489/abort_td()!
>>>>   BUG!
>>>>   ### ERROR ### Please RESET the board ###
>>>>
>>>> The cause of the error seems the following code:
>>>>
>>>>   #ifdef CONFIG_USB_EHCI_HCD
>>>>   /*
>>>>    * The U-Boot EHCI driver can handle any transfer length as long as there is
>>>>    * enough free heap space left, but the SCSI READ(10) and WRITE(10) commands are
>>>>    * limited to 65535 blocks.
>>>>    */
>>>>   #define USB_MAX_XFER_BLK    65535
>>>>   #else
>>>>   #define USB_MAX_XFER_BLK    20
>>>>   #endif
>>>>
>>>> To fix the problem, choose the chunk size at run-time for CONFIG_BLK.
>>>
>>> What happens if CONFIG_BLK is not set ?
>>
>> USB_MAX_XFER_BLK is chosen.
>
> And can we fix that even for non-CONFIG_BLK ?
>
>>> Why is it 20 for XHCI anyway ?
>>
>> You are the maintainer.
>> (I hope) you have better knowledge with this.
>
> Heh, way to deflect the question. I seem to remember some discussion
> about the DMA (?) limitation on XHCI, but I'd have to dig through the ML
> archives myself.
>
>> Looks like the following commit was picked up by you.
>
> 5 years ago, way before DM was what it is today .

And even way before the introduction of XHCI into U-Boot, which means
that this 20 was targeting OHCI or proprietary HCDs, not XHCI.
USB_MAX_READ_BLK was already set to 20 in the initial revision of
usb_storage.c. As I said in the commit message, this 20 was certainly
not optimal for these non-EHCI HCDs, but it restored the previous
(i.e. pre-5dd95cf) behavior for these HCDs instead of using the 5 * 4
KiB code, which was specific to ehci-hcd.c at that time. Without
knowing the rationale for the legacy 20 blocks, the safest approach
for non-EHCI HCDs was to use this value in order to avoid breaking a
platform or something. Looking at ohci-hcd.c, it limits the transfer
size to (N_URB_TD - 2) * 4 KiB, with N_URB_TD set to 48, so the
maximum number of transfers would depend on the MSC block size.
dwc2.c, isp116x-hcd.c, r8a66597-hcd.c, and sl811-hcd.c do not seem to
have any limit caused by these drivers. The limit with the current
XHCI code seems to be 64 * 64 KiB. So, nowadays, USB_MAX_XFER_BLK
could be set to 65535 for all HCDs but OHCI and XHCI, which require
specific rules depending on the MSC block size.

>> commit cffcc503580907d733e694d505e12ff6ec6c679a
>> Author:     Benoît Thébaudeau <benoit.thebaudeau at advansee.com>
>> AuthorDate: Fri Aug 10 18:26:50 2012 +0200
>> Commit:     Marek Vasut <marex at denx.de>
>> CommitDate: Sat Sep 1 16:21:52 2012 +0200
>>
>>     usb_storage: Restore non-EHCI support
>>
>>     The commit 5dd95cf made the MSC driver EHCI-specific. This patch restores a
>>     basic support of non-EHCI HCDs, like before that commit.
>>
>>     The fallback transfer size is certainly not optimal, but at least
>> it should work
>>     like before.
>>
>>     Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau at advansee.com>
>>     Cc: Marek Vasut <marex at denx.de>
>>     Cc: Ilya Yanok <ilya.yanok at cogentembedded.com>
>>     Cc: Stefan Herbrechtsmeier <stefan at herbrechtsmeier.net>
>>
>> diff --git a/common/usb_storage.c b/common/usb_storage.c
>> index bdc306f587fd..0cd6399a3c42 100644
>> --- a/common/usb_storage.c
>> +++ b/common/usb_storage.c
>> @@ -155,11 +155,15 @@ struct us_data {
>>         trans_cmnd      transport;              /* transport routine */
>>  };
>>
>> +#ifdef CONFIG_USB_EHCI
>>  /*
>>   * The U-Boot EHCI driver cannot handle more than 5 page aligned buffers
>>   * of 4096 bytes in a transfer without running itself out of qt_buffers
>>   */
>>  #define USB_MAX_XFER_BLK(start, blksz) (((4096 * 5) - (start % 4096)) / blksz)
>> +#else
>> +#define USB_MAX_XFER_BLK(start, blksz) 20
>> +#endif
>>
>>  static struct us_data usb_stor[USB_MAX_STOR_DEV];

Best regards,
Benoît