[U-Boot] CONFIG_USB_EHCI_HCD breaks xHCI drivers even on DM

[Simon Glass]

Hi,

On 4 July 2017 at 03:15, Masahiro Yamada <yamada.masahiro at socionext.com> wrote:
> Hi Marek,
>
>
> 2017-07-03 17:13 GMT+09:00 Marek Vasut <marex at 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