+Simon
2017-07-20 18:00 GMT+09:00 Marek Vasut marex@denx.de:
On 07/20/2017 09:49 AM, Masahiro Yamada wrote:
2017-07-20 2:33 GMT+09:00 Marek Vasut marex@denx.de:
On 07/19/2017 05:38 PM, Masahiro Yamada wrote:
2017-07-15 21:57 GMT+09:00 Marek Vasut marex@denx.de:
On 07/15/2017 01:30 AM, Benoît Thébaudeau wrote:
On Sat, Jul 15, 2017 at 12:06 AM, Marek Vasut marex@denx.de wrote: > On 07/14/2017 11:46 PM, Benoît Thébaudeau wrote: >> On Fri, Jul 14, 2017 at 1:50 PM, Marek Vasut marex@denx.de wrote: >>> On 07/14/2017 01:03 PM, Masahiro Yamada wrote: >>>> 2017-07-14 19:07 GMT+09:00 Marek Vasut marex@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. > > For whatever reason, something tells me that setting the block size to > 64k for XHCI broke things, but I cannot locate the thread. But there's > something in the back of my head ...
Indeed: according to what I said above, USB_MAX_XFER_BLK cannot be set to 65535 for XHCI. With an MSC block size of blksz = 512 bytes / block, USB_MAX_XFER_BLK can be set to at most 1 segment * (TRBS_PER_SEGMENT = 64 TRBs / segment) * (TRB_MAX_BUFF_SIZE = 65536 bytes / TRB) / blksz = 8192 blocks for XHCI. And for OHCI, the limit is (N_URB_TD - 2 = 46 TDs) * (4096 bytes / TD) / blksz = 368 blocks. The buffer alignment may also have to be taken into account to adjust these values, which would require a USB_MAX_XFER_BLK(host_if, start, blksz) macro or function. USB_MAX_XFER_BLK can however be set to 65535 regardless of blksz for all the other HCDs (i.e. EHCI, dwc2.c, isp116x-hcd.c, r8a66597-hcd.c, and sl811-hcd.c).
That's probably what I was looking for, thanks.
So, how shall we handle this?
If somebody can fix this in a correct way, I am happy to hand over this.
Any way to fix it for !CONFIG_BLK ?
common/usb_storage.c is sprinkled with ugly #ifdef CONFIG_BLK
IIUC, !CONFIG_BLK code will be removed after migration.
Is it worthwhile to save !CONFIG_BLK case?
Hmmmmmm, sigh. When is the migration happening, how far is it ?
Maybe, we can ask Simon for his plan.
I think there are currently three cases:
[1] CONFIG_DM_USB && CONFIG_BLK
[2] CONFIG_DM_USB && !CONFIG_BLK
[3] !CONFIG_DM_USB
IIUC, only [1] is future-proof. Meanwhile, the code is really dirty. I hope it will not last very long.