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

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

[Masahiro Yamada]

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?



-- 
Best Regards
Masahiro Yamada

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

[Marek Vasut]

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.

-- 
Best regards,
Marek Vasut

[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@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

[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@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