| Message ID | CAK7LNASMP-NWcfkmAH=0K7=XHFtQpm9Ezt9mfzHatW1ck6nn=w@mail.gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Marek Vasut |
| Headers | show |
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 . > commit cffcc503580907d733e694d505e12ff6ec6c679a > Author: Benoît Thébaudeau <benoit.thebaudeau@advansee.com> > AuthorDate: Fri Aug 10 18:26:50 2012 +0200 > Commit: Marek Vasut <marex@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@advansee.com> > Cc: Marek Vasut <marex@denx.de> > Cc: Ilya Yanok <ilya.yanok@cogentembedded.com> > Cc: Stefan Herbrechtsmeier <stefan@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]; > > > > > > >
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. >> commit cffcc503580907d733e694d505e12ff6ec6c679a >> Author: Benoît Thébaudeau <benoit.thebaudeau@advansee.com> >> AuthorDate: Fri Aug 10 18:26:50 2012 +0200 >> Commit: Marek Vasut <marex@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@advansee.com> >> Cc: Marek Vasut <marex@denx.de> >> Cc: Ilya Yanok <ilya.yanok@cogentembedded.com> >> Cc: Stefan Herbrechtsmeier <stefan@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
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 ... [...]
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). Best regards, Benoît
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.
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.
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 ?
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?
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 ?
+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.
+Simon, On Thu, Jul 20, 2017 at 5:00 PM, Marek Vasut <marex@denx.de> wrote: > 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 ? 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... Regards, Bin
On 07/20/2017 11:38 AM, Bin Meng wrote: > +Simon, > > On Thu, Jul 20, 2017 at 5:00 PM, Marek Vasut <marex@denx.de> wrote: >> 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 ? > > 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 :-)
+Tom for question below Hi, On 20 July 2017 at 03:40, Marek Vasut <marex@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 <marex@denx.de> wrote: >>> 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 ? >> >> 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? > > -- > Best regards, > Marek Vasut
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 <marex@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 <marex@denx.de> wrote: > >>> 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 ? > >> > >> 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?
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];