[U-Boot] [U-Boot, v3, 3/8] usb: hub: Power-cycle on root-hub ports

[Vivek Gautam <gautamvivek1987@gmail.com>]

HI Julius,


On Sat, Apr 20, 2013 at 12:30 AM, Julius Werner <jwerner@chromium.org> wrote:
>> XHCI ports are powered on after a H/W reset, however
>> EHCI ports are not. So disabling and re-enabling power
>> on all ports invariably.
>>
>> Signed-off-by: Amar <amarendra.xt@samsung.com>
>> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
>>
>> ---
>> Changes from v2:
>>  - Replaced USB_HUB_PRINTFs to debug()
>>
>>  common/usb_hub.c |   34 ++++++++++++++++++++++++++++++++++
>>  1 files changed, 34 insertions(+), 0 deletions(-)
>>
>> diff --git a/common/usb_hub.c b/common/usb_hub.c
>> index f2a0285..e4f4e3c 100644
>> --- a/common/usb_hub.c
>> +++ b/common/usb_hub.c
>> @@ -100,11 +100,45 @@ static void usb_hub_power_on(struct usb_hub_device *hub)
>>       int i;
>>       struct usb_device *dev;
>>       unsigned pgood_delay = hub->desc.bPwrOn2PwrGood * 2;
>> +     ALLOC_CACHE_ALIGN_BUFFER(struct usb_port_status, portsts, 1);
>> +     unsigned short portstatus;
>> +     int ret;
>>
>>       dev = hub->pusb_dev;
>>       /* Enable power to the ports */
>>       debug("enabling power on all ports\n");
>>       for (i = 0; i < dev->maxchild; i++) {
>> +             /*
>> +              * Power-cycle the ports here: aka,
>> +              * turning them off and turning on again.
>> +              */
>> +             usb_clear_port_feature(dev, i + 1, USB_PORT_FEAT_POWER);
>> +             debug("port %d returns %lX\n", i + 1, dev->status);
>> +
>> +             /* Wait at least 2*bPwrOn2PwrGood for PP to change */
>> +             mdelay(pgood_delay);
>
> This adds 20ms per port to USB boot times... is it really necessary? Why do we
> need to powercycle XHCI ports? Couldn't we just check if the ports are powered
> and only power them on if they aren't?

This 20ms delay is truely being taken to be on safer side (twice of
Power-on-to-power-good),
its not observational.
In my earlier patch-series, we were doing the same way as you are
suggesting here (power-on
ports only if they aren't), however Marek suggested to power-cycle the
ports. This would ensure
that we don't have any spurious Port status (telling us that port is
powered up).

Actually i was seeing a strage behavior on USB 2.0 protocol ports
available with XHCI.
Since all ports with XHCI are powered-up after a Chip-reset, the
instant we do a power-on
on these ports (as with original code - simply setting the PORT_POWER
feature), the Connect status
change bit used to get cleared (however Current connect status bit was
still set).

So we arrived at solution that either we don't power-up XHCI ports or
do a power-cycle on them.

>
> If you insist, please at least parallelize this. Disable power on all ports,
> wait, then check and reenable it.

Hmm, so right now we are doing this for one port at a time.
I am sure parallelising things as you suggested would be best to do here, but
can you please explain, would handling one port at a time lead to unwanted
behavior from Host's side somehow ?

>
>> +
>> +             ret = usb_get_port_status(dev, i + 1, portsts);
>> +             if (ret < 0) {
>> +                     debug("port %d: get_port_status failed\n", i + 1);
>
> This is a severe error (because the ports won't come up again), so I think it
> should be a printf() instead of a debug().

Sure, will change this to pritnf()

>
>> +                     return;
>> +             }
>> +
>> +             /*
>> +              * Check to confirm the state of Port Power:
>> +              * xHCI says "After modifying PP, s/w shall read
>> +              * PP and confirm that it has reached the desired state
>> +              * before modifying it again, undefined behavior may occur
>> +              * if this procedure is not followed".
>> +              * EHCI doesn't say anything like this, but no harm in keeping
>> +              * this.
>> +              */
>> +             portstatus = le16_to_cpu(portsts->wPortStatus);
>> +             if (portstatus & (USB_PORT_STAT_POWER << 1)) {
>> +                     debug("port %d: Port power change failed\n", i + 1);
>
> Same as above.

Sure, will change this.

>
>> +                     return;
>> +             }
>> +
>>               usb_set_port_feature(dev, i + 1, USB_PORT_FEAT_POWER);
>>               debug("port %d returns %lX\n", i + 1, dev->status);
>>       }



-- 
Best Regards
Vivek