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

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(-)

> 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?

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

> +
> +		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().

> +			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.

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

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);
>>       }

> 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).

Fair enough... I guess 20ms overall is not a big deal in the whole
picture. I sometimes tend to overoptimize things...

> 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).

This is a bug in your XHCI code I hadn't spotted before: in
xhci_submit_root(SET_FEATURE) you read the PORTSC register, add a bit,
and write it again... without calling xhci_port_state_to_neutral()
inbetween. Thus you clear any pending change events when you set
PORT_POWER. (Seems the EHCI code has a similar bug in CLEAR_FEATURE,
now that I'm looking at it... someone should put a 'reg &=
~EHCI_PS_CLEAR;' in there.)

> 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 ?

It doesn't affect correctness, just the amount of time "wasted". Doing
it one port at a time means you wait 100ms on a 5 port root hub, while
you could get by with 20ms overall by parallelizing it.

Hi,


On Tue, Apr 23, 2013 at 3:32 AM, Julius Werner <jwerner@chromium.org> wrote:
>> 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).
>
> Fair enough... I guess 20ms overall is not a big deal in the whole
> picture. I sometimes tend to overoptimize things...
>
>> 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).
>
> This is a bug in your XHCI code I hadn't spotted before: in
> xhci_submit_root(SET_FEATURE) you read the PORTSC register, add a bit,
> and write it again... without calling xhci_port_state_to_neutral()
> inbetween. Thus you clear any pending change events when you set
> PORT_POWER.

Right, we need to do that.

> (Seems the EHCI code has a similar bug in CLEAR_FEATURE,
> now that I'm looking at it... someone should put a 'reg &=
> ~EHCI_PS_CLEAR;' in there.)

True, EHCI has it for SET_FEATURE but not for CLEAR_FEATURE.

>
>> 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 ?
>
> It doesn't affect correctness, just the amount of time "wasted". Doing
> it one port at a time means you wait 100ms on a 5 port root hub, while
> you could get by with 20ms overall by parallelizing it.

True, will amend this as required.