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

[Masahiro Yamada]

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.



> 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