[U-Boot] [U-Boot, v3, 3/8] usb: hub: Power-cycle on root-hub ports
Vivek Gautam
gautamvivek1987 at gmail.com
Mon Apr 22 10:21:48 CEST 2013
HI Julius,
On Sat, Apr 20, 2013 at 12:30 AM, Julius Werner <jwerner at 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 at samsung.com>
>> Signed-off-by: Vivek Gautam <gautam.vivek at 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
More information about the U-Boot
mailing list