On Fri, Jul 21, 2017 at 04:48:58AM -0600, Simon Glass wrote: > +Tom for question below > > Hi, > > On 20 July 2017 at 03:40, Marek Vasut <ma...@denx.de> wrote: > > On 07/20/2017 11:38 AM, Bin Meng wrote: > >> +Simon, > >> > >> On Thu, Jul 20, 2017 at 5:00 PM, Marek Vasut <ma...@denx.de> wrote: > >>> On 07/20/2017 09:49 AM, Masahiro Yamada wrote: > >>>> 2017-07-20 2:33 GMT+09:00 Marek Vasut <ma...@denx.de>: > >>>>> On 07/19/2017 05:38 PM, Masahiro Yamada wrote: > >>>>>> 2017-07-15 21:57 GMT+09:00 Marek Vasut <ma...@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 <ma...@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 <ma...@denx.de> wrote: > >>>>>>>>>>> On 07/14/2017 01:03 PM, Masahiro Yamada wrote: > >>>>>>>>>>>> 2017-07-14 19:07 GMT+09:00 Marek Vasut <ma...@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 ? > >> > >> One idea is to force all board to switch to driver model at a preset > >> timeline. After the deadline, boards do not switch to DM will get > >> dropped by the mainline. I noticed that not all boards are actively > >> maintained... > > > > Be my guest, there's a few which I'd like to see removed myself :-) > > That makes sense although I'm not sure what the deadline should be. > CONFIG_BLK is invasive and it is a pain to carry the #ifdefs. > > Maybe end of year, or is that too short?
9 months, rounded up to next release? -- Tom
signature.asc
Description: Digital signature
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
