| Message ID | CAK7LNASxoSYC+oxX7A2NzUTQrWjevVpxJDi5ty6qC9fU2JXeQA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On 07/03/2017 08:12 AM, Masahiro Yamada wrote: > Hi Marek, Simon. > > > Basically, Driver Mode allows us to enable multiple drivers without > affecting each other > because drivers are configured in probe functions > instead of compile-time configuration by #ifdef:s > > With DM, I think it is legitimate to enable EHCI and xHCI at the same time, > but it is not actually true. > > > If CONFIG_USB_EHCI_HCD is disabled, xHCI drivers work fine, > but with CONFIG_USB_EHCI_HCD enabled, xHCI drivers go wrong as follows: > > => fatload usb 0 82000000 Image > reading Image > WARN halted endpoint, queueing URB anyway. > Unexpected XHCI event TRB, skipping... (3fb54090 00000001 13000000 01008401) > BUG: failure at > /home/masahiro/workspace/u-boot-yamada/drivers/usb/host/xhci-ring.c:489/abort_td()! > BUG! > ### ERROR ### Please RESET the board ### > > > Here, "Image" is larger than 10MB. > > > > I narrowed down the cause of the problem > in the following code in common/usb_storage.c > > > #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 > > > > Of course, this ifdef is not acceptable in Driver Model, > so we need to fix it somehow. > > > I am not familiar with that part at all, > but I just aligned the value to the lowest common denominator (20) > as follows: > > > > diff --git a/common/usb_storage.c b/common/usb_storage.c > index 03171f74cb02..b6d16e3dead3 100644 > --- a/common/usb_storage.c > +++ b/common/usb_storage.c > @@ -100,16 +100,7 @@ struct us_data { > trans_cmnd transport; /* transport routine */ > }; > > -#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 > > #ifndef CONFIG_BLK > static struct us_data usb_stor[USB_MAX_STOR_DEV]; > > > > With with, both EHCI and xHCI seem to work > but I am not sure if this is the right way to fix the problem. > > Thought? You need to set the maximum blocksize based on whether the controller communicating with the storage device is USB 2.0 or 3.0 , which should be possible to extract from the info provided by DM. Even better would be to figure out why the xhci needs this 20 blocks limit and fix it though.
Hi Marek, 2017-07-03 17:13 GMT+09:00 Marek Vasut <marex@denx.de>: > On 07/03/2017 08:12 AM, Masahiro Yamada wrote: >> Hi Marek, Simon. >> >> >> Basically, Driver Mode allows us to enable multiple drivers without >> affecting each other >> because drivers are configured in probe functions >> instead of compile-time configuration by #ifdef:s >> >> With DM, I think it is legitimate to enable EHCI and xHCI at the same time, >> but it is not actually true. >> >> >> If CONFIG_USB_EHCI_HCD is disabled, xHCI drivers work fine, >> but with CONFIG_USB_EHCI_HCD enabled, xHCI drivers go wrong as follows: >> >> => fatload usb 0 82000000 Image >> reading Image >> WARN halted endpoint, queueing URB anyway. >> Unexpected XHCI event TRB, skipping... (3fb54090 00000001 13000000 01008401) >> BUG: failure at >> /home/masahiro/workspace/u-boot-yamada/drivers/usb/host/xhci-ring.c:489/abort_td()! >> BUG! >> ### ERROR ### Please RESET the board ### >> >> >> Here, "Image" is larger than 10MB. >> >> >> >> I narrowed down the cause of the problem >> in the following code in common/usb_storage.c >> >> >> #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 >> >> >> >> Of course, this ifdef is not acceptable in Driver Model, >> so we need to fix it somehow. >> >> >> I am not familiar with that part at all, >> but I just aligned the value to the lowest common denominator (20) >> as follows: >> >> >> >> diff --git a/common/usb_storage.c b/common/usb_storage.c >> index 03171f74cb02..b6d16e3dead3 100644 >> --- a/common/usb_storage.c >> +++ b/common/usb_storage.c >> @@ -100,16 +100,7 @@ struct us_data { >> trans_cmnd transport; /* transport routine */ >> }; >> >> -#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 >> >> #ifndef CONFIG_BLK >> static struct us_data usb_stor[USB_MAX_STOR_DEV]; >> >> >> >> With with, both EHCI and xHCI seem to work >> but I am not sure if this is the right way to fix the problem. >> >> Thought? > > You need to set the maximum blocksize based on whether the controller > communicating with the storage device is USB 2.0 or 3.0 , which should > be possible to extract from the info provided by DM. Thanks for your advise. Could you tell me how to do it? Also, your comment sounds like the current implementation is already crappy, but I can keep it as follows if you prefer. /* REVISIT: why chunk size is different? */ max_block = usb_is_ehci(dev) ? 65535 : 20; I do not know how to make usb_is_ehci(), though. > Even better would be to figure out why the xhci needs this 20 blocks > limit and fix it though. If I had plenty of time, I could take a look into it. However, it does not sound fair to require it to fix the current problem.
Hi, On 4 July 2017 at 03:15, Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > Hi Marek, > > > 2017-07-03 17:13 GMT+09:00 Marek Vasut <marex@denx.de>: >> On 07/03/2017 08:12 AM, Masahiro Yamada wrote: >>> Hi Marek, Simon. >>> >>> >>> Basically, Driver Mode allows us to enable multiple drivers without >>> affecting each other >>> because drivers are configured in probe functions >>> instead of compile-time configuration by #ifdef:s >>> >>> With DM, I think it is legitimate to enable EHCI and xHCI at the same time, >>> but it is not actually true. >>> >>> >>> If CONFIG_USB_EHCI_HCD is disabled, xHCI drivers work fine, >>> but with CONFIG_USB_EHCI_HCD enabled, xHCI drivers go wrong as follows: >>> >>> => fatload usb 0 82000000 Image >>> reading Image >>> WARN halted endpoint, queueing URB anyway. >>> Unexpected XHCI event TRB, skipping... (3fb54090 00000001 13000000 01008401) >>> BUG: failure at >>> /home/masahiro/workspace/u-boot-yamada/drivers/usb/host/xhci-ring.c:489/abort_td()! >>> BUG! >>> ### ERROR ### Please RESET the board ### >>> >>> >>> Here, "Image" is larger than 10MB. >>> >>> >>> >>> I narrowed down the cause of the problem >>> in the following code in common/usb_storage.c >>> >>> >>> #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 >>> >>> >>> >>> Of course, this ifdef is not acceptable in Driver Model, >>> so we need to fix it somehow. >>> >>> >>> I am not familiar with that part at all, >>> but I just aligned the value to the lowest common denominator (20) >>> as follows: >>> >>> >>> >>> diff --git a/common/usb_storage.c b/common/usb_storage.c >>> index 03171f74cb02..b6d16e3dead3 100644 >>> --- a/common/usb_storage.c >>> +++ b/common/usb_storage.c >>> @@ -100,16 +100,7 @@ struct us_data { >>> trans_cmnd transport; /* transport routine */ >>> }; >>> >>> -#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 >>> >>> #ifndef CONFIG_BLK >>> static struct us_data usb_stor[USB_MAX_STOR_DEV]; >>> >>> >>> >>> With with, both EHCI and xHCI seem to work >>> but I am not sure if this is the right way to fix the problem. >>> >>> Thought? >> >> You need to set the maximum blocksize based on whether the controller >> communicating with the storage device is USB 2.0 or 3.0 , which should >> be possible to extract from the info provided by DM. > > > Thanks for your advise. > Could you tell me how to do it? > > Also, your comment sounds like the current implementation is already > crappy, but I can keep it as follows if you prefer. > > /* REVISIT: why chunk size is different? */ > max_block = usb_is_ehci(dev) ? 65535 : 20; > > I do not know how to make usb_is_ehci(), though. You can perhaps add a new field to the private USB structure and set it to different values for each stack. > > > >> Even better would be to figure out why the xhci needs this 20 blocks >> limit and fix it though. > > If I had plenty of time, I could take a look into it. > However, it does not sound fair to require it > to fix the current problem. > > -- > Best Regards > Masahiro Yamada Regards, Simon
diff --git a/common/usb_storage.c b/common/usb_storage.c index 03171f74cb02..b6d16e3dead3 100644 --- a/common/usb_storage.c +++ b/common/usb_storage.c @@ -100,16 +100,7 @@ struct us_data { trans_cmnd transport; /* transport routine */ }; -#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 #ifndef CONFIG_BLK