[U-Boot] [PATCH 7/7] usb: hub: Increase the query delay

[Stefan Roese]

On 04.05.2016 13:39, Marek Vasut wrote:
> On 05/04/2016 10:07 AM, Stefan Roese wrote:
>> On 03.05.2016 22:51, Marek Vasut wrote:
>>> Increase the query delay, otherwise some sticks are not detected.
>>> The problem shows up on the USB bus analyzer such that the stick
>>> takes longer time to switch from FS mode to HS mode than the code
>>> allows.
>>>
>>> Signed-off-by: Marek Vasut <marex at denx.de>
>>> Cc: Chin Liang See <clsee at altera.com>
>>> Cc: Dinh Nguyen <dinguyen at opensource.altera.com>
>>> Cc: Hans de Goede <hdegoede at redhat.com>
>>> Cc: Stefan Roese <sr at denx.de>
>>> Cc: Stephen Warren <swarren at nvidia.com>
>>> ---
>>>    common/usb_hub.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/common/usb_hub.c b/common/usb_hub.c
>>> index 0f39c9f..6cd274a 100644
>>> --- a/common/usb_hub.c
>>> +++ b/common/usb_hub.c
>>> @@ -145,7 +145,7 @@ static void usb_hub_power_on(struct usb_hub_device
>>> *hub)
>>>         * Do a minimum delay of the larger value of 100ms or pgood_delay
>>>         * so that the power can stablize before the devices are queried
>>>         */
>>> -    hub->query_delay = get_timer(0) + max(100, (int)pgood_delay);
>>> +    hub->query_delay = get_timer(0) + max(1000, (int)pgood_delay);
>>>
>>>        /*
>>>         * Record the power-on timeout here. The max. delay (timeout)
>>>
>>
>> We have touched this part of the delay a number of times in my
>> USB scanning patch series. I've integrated all very valuable
>> suggestions from Stephen and Hans and I'm pretty sure that the
>> current implementation is aligned with the USB spec.
>
> Sadly, not everyone goes exactly be the USB spec it seems.
> Especially the cheap sticks which are the majority in the market.
>
>> And tested
>> successfully one multiple platforms without regression.
>> Stephen did some tests on Tegra and Hans on sunxi. I tested
>> mainly on x86. But I now also tested on Armada XP (see
>> below).
>
> All of which use the same EHCI or XHCI controller, correct ?
>
>> As mentioned before, the current version causes no problems with
>> all my USB sticks on the congatec x86 board. Even the 2 ones that
>> are problematic when connected to the SoCFPGA. They are detected
>> without any issues on this board. Thats why I assume, that the
>> real problem here is the DWC driver and not the generic USB
>> handling code.
>
> The logic here is flawed. If the code works against EHCI controller and
> does not work against DWC2 controller, you cannot infer from this that
> the DWC2 controller is the problem.
>
> This can also be specific behavior of the EHCI controller, and I in fact
> suspect it is, but you cannot decide this without checking the
> bus itself.
>
>> My feeling is that increasing this initial delay
>> (before the scanning starts) just papers over the real problem
>> most likely hidden somewhere in the DWC driver. This is just
>> a feeling though which I can't prove somehow other than testing
>> the common USB code on different platforms. And I really have
>> no deeper knowledge of the DWC driver to manifest this feeling
>> or even fix this potential problem there.
>
> The bus analyzer tells me that the stick just takes longer to come out
> of FS mode and switch to HS mode when the USB started from reset state
> of the controller, I decide to trust what the analyzer shows me instead.
>
> See attached logs.
>
>> I've already mentioned this in another mail. My suggestion for
>> SoCFPGA boards that need to use these problematic USB keys is
>> to use the already available solution to set "usb_pgood_delay"
>> to 1000. This effectively does the same as this patch. Without
>> implying this general 1 second delay per hub (!!!) to all other
>> platforms that use USB in U-Boot.
>>
>> To test my suspicions about this being a DWC (SoCFPGA?) only
>> problem, I've also tested all my current USB sticks including
>> the 2 problematic ones (on SoCrates) on another ARM platform
>> (additionally to all my test on x86). I've used the Marvell
>> Armada XP development board (db-mv784mp-gp) for this. And all
>> USB sticks are detected without any problems on this platform.
>
> I see, it would be interesting to know what happens on the bus on the
> marvell board compared to the socfpga board. For the socfpga, the bus
> starts from complete cold state, could it be the marvell does have the
> EHCI running when you do your tests ? That would explain why the stick
> might be ready much faster on your platform.
>
>> As a result of all this, I would like to have this patch not
>> applied. As it negatively touches the common USB code to fix
>> (paper over?) a problem only seen on one platform (AFAIK). And
>> we already have the solution of this "usb_pgood_delay" that
>> can be used on SoCFPGA. To manifest this, here again the
>> numbers for the USB scanning time on x86, without and with
>> this patch:
>>
>> Without this patch:
>> starting USB...
>> USB0:   USB EHCI 1.00
>> scanning bus 0 for devices... 9 USB Device(s) found
>>
>> time: 1.940 seconds
>>
>> With this patch:
>> => time usb start
>> starting USB...
>> USB0:   USB EHCI 1.00
>> scanning bus 0 for devices... 9 USB Device(s) found
>>
>> time: 5.421 seconds
>
> Well that's great, removing some delays makes the code run faster and
> breaks some platform. Stefan, I need a solution for this release and
> the release is coming very quickly. So far, I see no other proposals
> how to fix this issue and it's been reported for well over a month.
>
> As I don't want to have regressions in this release, here are the two
> options:
> a) I revert "usb: Change power-on / scanning timeout handling"
> b) I apply these 7 patches. I am willing to isolate this increase
>     of delay with an ifdef to DWC2 controller, but that does NOT
>     seem correct according to the analyzer. I would just wait for
>     an EHCI controller on which this breaks.

You are missing c), use "usb_pgood_delay" here. But okay, please use
option b) for this release. We can always re-visit this issue later
and try to find a "better" way to fix this.

Thanks,
Stefan