[U-Boot] [PATCH 1/2] usb: hub: fix power good delay timing

[U-Boot] [PATCH 1/2] usb: hub: fix power good delay timing

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

usb_hub_power_on() currently waits for the maximum of (a) the hub port's
power output to become good, (b) the max time the USB specification
allows a device to take to connect.

However, these two operations must occur in series rather than in
parallel. First, the power supply ramps up to the level required to
power the USB device, and then the device may take a certain amount of
time to connect (assert D+/D- pullups).

Related, the maximum time that a device has to assert pullups is 1s not
100ms.

This is explained in "Connect Timing ECN.pdf", itself part of
usb_20_042814.zip from www.usb.org.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 common/usb_hub.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/common/usb_hub.c b/common/usb_hub.c
index ffac0e743cef..4875a51aceaf 100644
--- a/common/usb_hub.c
+++ b/common/usb_hub.c
@@ -36,7 +36,7 @@
 #endif
 
 #ifndef CONFIG_USB_HUB_MIN_POWER_ON_DELAY
-#define CONFIG_USB_HUB_MIN_POWER_ON_DELAY	100
+#define CONFIG_USB_HUB_MIN_POWER_ON_DELAY	1000
 #endif
 
 #define USB_BUFSIZ	512
@@ -138,8 +138,11 @@ static void usb_hub_power_on(struct usb_hub_device *hub)
 		debug("port %d returns %lX\n", i + 1, dev->status);
 	}
 
-	/* Wait for power to become stable */
-	mdelay(max(pgood_delay, CONFIG_USB_HUB_MIN_POWER_ON_DELAY));
+	/*
+	 * Wait for power to become stable,
+	 * plus spec-defined max time for device to connect
+	 */
+	mdelay(pgood_delay + CONFIG_USB_HUB_MIN_POWER_ON_DELAY);
 }
 
 void usb_hub_reset(void)
-- 
1.8.1.5

[U-Boot] [PATCH 2/2] usb: hub: remove CONFIG_USB_HUB_MIN_POWER_ON_DELAY

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

Now that we wait the correct specification-mandated time at the end of
usb_hub_power_on(), I suspect that CONFIG_USB_HUB_MIN_POWER_ON_DELAY has
no purpose.

For cm_t35.h, we already wait longer than the original MIN_POWER_ON_DELAY,
so this change is safe.

For gw_ventana.h, we will wait as long as the original MIN_POWER_ON_DELAY
iff pgood_delay was at least 200ms. I'm not sure if this is the case or
not, hence I've CC'd relevant people to test this change.

Cc: Igor Grinberg <grinberg@compulab.co.il>
Cc: Tim Harvey <tharvey@gateworks.com>
Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 README                       | 3 ---
 common/usb_hub.c             | 6 +-----
 include/configs/cm_t35.h     | 2 --
 include/configs/gw_ventana.h | 1 -
 4 files changed, 1 insertion(+), 11 deletions(-)

diff --git a/README b/README
index d8fcd95f9423..091897227c52 100644
--- a/README
+++ b/README
@@ -1432,9 +1432,6 @@ The following options need to be configured:
 		CONFIG_USB_EHCI_TXFIFO_THRESH enables setting of the
 		txfilltuning field in the EHCI controller on reset.
 
-		CONFIG_USB_HUB_MIN_POWER_ON_DELAY defines the minimum
-		interval for usb hub power-on delay.(minimum 100msec)
-
 - USB Device:
 		Define the below if you wish to use the USB console.
 		Once firmware is rebuilt from a serial console issue the
diff --git a/common/usb_hub.c b/common/usb_hub.c
index 4875a51aceaf..2add4b97920f 100644
--- a/common/usb_hub.c
+++ b/common/usb_hub.c
@@ -35,10 +35,6 @@
 #include <asm/4xx_pci.h>
 #endif
 
-#ifndef CONFIG_USB_HUB_MIN_POWER_ON_DELAY
-#define CONFIG_USB_HUB_MIN_POWER_ON_DELAY	1000
-#endif
-
 #define USB_BUFSIZ	512
 
 static struct usb_hub_device hub_dev[USB_MAX_HUB];
@@ -142,7 +138,7 @@ static void usb_hub_power_on(struct usb_hub_device *hub)
 	 * Wait for power to become stable,
 	 * plus spec-defined max time for device to connect
 	 */
-	mdelay(pgood_delay + CONFIG_USB_HUB_MIN_POWER_ON_DELAY);
+	mdelay(pgood_delay + 1000);
 }
 
 void usb_hub_reset(void)
diff --git a/include/configs/cm_t35.h b/include/configs/cm_t35.h
index aae05e033303..f6acf6920524 100644
--- a/include/configs/cm_t35.h
+++ b/include/configs/cm_t35.h
@@ -104,8 +104,6 @@
 #define CONFIG_USB_DEVICE
 #define CONFIG_USB_TTY
 #define CONFIG_SYS_CONSOLE_IS_IN_ENV
-/* This delay is really for slow-to-power-on USB sticks, not the hub */
-#define CONFIG_USB_HUB_MIN_POWER_ON_DELAY 500
 
 /* commands to include */
 #include <config_cmd_default.h>
diff --git a/include/configs/gw_ventana.h b/include/configs/gw_ventana.h
index 33983907f228..188871630bd3 100644
--- a/include/configs/gw_ventana.h
+++ b/include/configs/gw_ventana.h
@@ -188,7 +188,6 @@
 #define CONFIG_USB_ETH_CDC
 #define CONFIG_NETCONSOLE
 #define CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP
-#define CONFIG_USB_HUB_MIN_POWER_ON_DELAY 1200
 
 /* serial console (ttymxc1,115200) */
 #define CONFIG_CONS_INDEX              1
-- 
1.8.1.5

[U-Boot] [PATCH 1/2] usb: hub: fix power good delay timing

[Stephen Warren]

On 05/19/2014 02:21 PM, Stephen Warren wrote:
> From: Stephen Warren <swarren@nvidia.com>
> 
> usb_hub_power_on() currently waits for the maximum of (a) the hub port's
> power output to become good, (b) the max time the USB specification
> allows a device to take to connect.
> 
> However, these two operations must occur in series rather than in
> parallel. First, the power supply ramps up to the level required to
> power the USB device, and then the device may take a certain amount of
> time to connect (assert D+/D- pullups).
> 
> Related, the maximum time that a device has to assert pullups is 1s not
> 100ms.
> 
> This is explained in "Connect Timing ECN.pdf", itself part of
> usb_20_042814.zip from www.usb.org.

Marek, does this series look OK?

[U-Boot] [PATCH 1/2] usb: hub: fix power good delay timing

[Marek Vasut]

On Thursday, May 29, 2014 at 11:25:32 PM, Stephen Warren wrote:
> On 05/19/2014 02:21 PM, Stephen Warren wrote:
> > From: Stephen Warren <swarren@nvidia.com>
> > 
> > usb_hub_power_on() currently waits for the maximum of (a) the hub port's
> > power output to become good, (b) the max time the USB specification
> > allows a device to take to connect.
> > 
> > However, these two operations must occur in series rather than in
> > parallel. First, the power supply ramps up to the level required to
> > power the USB device, and then the device may take a certain amount of
> > time to connect (assert D+/D- pullups).
> > 
> > Related, the maximum time that a device has to assert pullups is 1s not
> > 100ms.
> > 
> > This is explained in "Connect Timing ECN.pdf", itself part of
> > usb_20_042814.zip from www.usb.org.
> 
> Marek, does this series look OK?

Applied both, thanks!

Best regards,
Marek Vasut

[U-Boot] [PATCH 2/2] usb: hub: remove CONFIG_USB_HUB_MIN_POWER_ON_DELAY

[Tim Harvey]

On Mon, May 19, 2014 at 1:21 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> From: Stephen Warren <swarren@nvidia.com>
>
> Now that we wait the correct specification-mandated time at the end of
> usb_hub_power_on(), I suspect that CONFIG_USB_HUB_MIN_POWER_ON_DELAY has
> no purpose.
>
> For cm_t35.h, we already wait longer than the original MIN_POWER_ON_DELAY,
> so this change is safe.
>
> For gw_ventana.h, we will wait as long as the original MIN_POWER_ON_DELAY
> iff pgood_delay was at least 200ms. I'm not sure if this is the case or
> not, hence I've CC'd relevant people to test this change.
>
> Cc: Igor Grinberg <grinberg@compulab.co.il>
> Cc: Tim Harvey <tharvey@gateworks.com>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
>  README                       | 3 ---
>  common/usb_hub.c             | 6 +-----
>  include/configs/cm_t35.h     | 2 --
>  include/configs/gw_ventana.h | 1 -
>  4 files changed, 1 insertion(+), 11 deletions(-)
>
> diff --git a/README b/README
> index d8fcd95f9423..091897227c52 100644
> --- a/README
> +++ b/README
> @@ -1432,9 +1432,6 @@ The following options need to be configured:
>                 CONFIG_USB_EHCI_TXFIFO_THRESH enables setting of the
>                 txfilltuning field in the EHCI controller on reset.
>
> -               CONFIG_USB_HUB_MIN_POWER_ON_DELAY defines the minimum
> -               interval for usb hub power-on delay.(minimum 100msec)
> -
>  - USB Device:
>                 Define the below if you wish to use the USB console.
>                 Once firmware is rebuilt from a serial console issue the
> diff --git a/common/usb_hub.c b/common/usb_hub.c
> index 4875a51aceaf..2add4b97920f 100644
> --- a/common/usb_hub.c
> +++ b/common/usb_hub.c
> @@ -35,10 +35,6 @@
>  #include <asm/4xx_pci.h>
>  #endif
>
> -#ifndef CONFIG_USB_HUB_MIN_POWER_ON_DELAY
> -#define CONFIG_USB_HUB_MIN_POWER_ON_DELAY      1000
> -#endif
> -
>  #define USB_BUFSIZ     512
>
>  static struct usb_hub_device hub_dev[USB_MAX_HUB];
> @@ -142,7 +138,7 @@ static void usb_hub_power_on(struct usb_hub_device *hub)
>          * Wait for power to become stable,
>          * plus spec-defined max time for device to connect
>          */
> -       mdelay(pgood_delay + CONFIG_USB_HUB_MIN_POWER_ON_DELAY);
> +       mdelay(pgood_delay + 1000);
>  }
>
>  void usb_hub_reset(void)
> diff --git a/include/configs/cm_t35.h b/include/configs/cm_t35.h
> index aae05e033303..f6acf6920524 100644
> --- a/include/configs/cm_t35.h
> +++ b/include/configs/cm_t35.h
> @@ -104,8 +104,6 @@
>  #define CONFIG_USB_DEVICE
>  #define CONFIG_USB_TTY
>  #define CONFIG_SYS_CONSOLE_IS_IN_ENV
> -/* This delay is really for slow-to-power-on USB sticks, not the hub */
> -#define CONFIG_USB_HUB_MIN_POWER_ON_DELAY 500
>
>  /* commands to include */
>  #include <config_cmd_default.h>
> diff --git a/include/configs/gw_ventana.h b/include/configs/gw_ventana.h
> index 33983907f228..188871630bd3 100644
> --- a/include/configs/gw_ventana.h
> +++ b/include/configs/gw_ventana.h
> @@ -188,7 +188,6 @@
>  #define CONFIG_USB_ETH_CDC
>  #define CONFIG_NETCONSOLE
>  #define CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP
> -#define CONFIG_USB_HUB_MIN_POWER_ON_DELAY 1200
>
>  /* serial console (ttymxc1,115200) */
>  #define CONFIG_CONS_INDEX              1
> --
> 1.8.1.5
>

Stephen,

Sorry for the late reply - I'm just getting around to testing this
(and I realize that Marek already committed it which is ok by me).

I have a variety of USB Mass Storage devices that I tested when I was
looking at this and out of about 12 devices I found I had 1 'usb
stick' that requires 2100ms in order to respond and be successfully
scanned: 048d:1327 Integrated Technology Express, Inc 32GB USB stick.
I also found that rotational media (ie Seagate and Western Digital USB
drives) would not respond in 1000ms either which didn't surprise me as
I figured they needed some extra spin-up time. For all other devices I
had I found that 1000ms was adequate.

So do these devices I mention simply violate the USB spec?

I wonder if the delay should be able to be overridden with an env var
or an argument to 'usb start' to account for devices like this?

Regards,

Tim

[U-Boot] [PATCH 2/2] usb: hub: remove CONFIG_USB_HUB_MIN_POWER_ON_DELAY

[Stephen Warren]

On 06/03/2014 10:17 AM, Tim Harvey wrote:
> On Mon, May 19, 2014 at 1:21 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> From: Stephen Warren <swarren@nvidia.com>
>>
>> Now that we wait the correct specification-mandated time at the end of
>> usb_hub_power_on(), I suspect that CONFIG_USB_HUB_MIN_POWER_ON_DELAY has
>> no purpose.
>>
>> For cm_t35.h, we already wait longer than the original MIN_POWER_ON_DELAY,
>> so this change is safe.
>>
>> For gw_ventana.h, we will wait as long as the original MIN_POWER_ON_DELAY
>> iff pgood_delay was at least 200ms. I'm not sure if this is the case or
>> not, hence I've CC'd relevant people to test this change.

>> diff --git a/include/configs/gw_ventana.h b/include/configs/gw_ventana.h

>> -#define CONFIG_USB_HUB_MIN_POWER_ON_DELAY 1200

> Stephen,
> 
> Sorry for the late reply - I'm just getting around to testing this
> (and I realize that Marek already committed it which is ok by me).
> 
> I have a variety of USB Mass Storage devices that I tested when I was
> looking at this and out of about 12 devices I found I had 1 'usb
> stick' that requires 2100ms in order to respond and be successfully

Was that 2100ms or 1200ms? I ask because gw_ventana.h only had 1200, so
if devices require 2100ms then they presumably didn't work with the code
before my patch?

> scanned: 048d:1327 Integrated Technology Express, Inc 32GB USB stick.
> I also found that rotational media (ie Seagate and Western Digital USB
> drives) would not respond in 1000ms either which didn't surprise me as
> I figured they needed some extra spin-up time. For all other devices I
> had I found that 1000ms was adequate.
> 
> So do these devices I mention simply violate the USB spec?

I *think* so yes.

> I wonder if the delay should be able to be overridden with an env var
> or an argument to 'usb start' to account for devices like this?

Yes, perhaps it is worth U-Boot probing for longer than the minimum
time, either always or on-demand as requested by an environment
variable. The only downside would be that "usb start" would take longer
even in the absence of these broken(?) devices if we always delay
longer, rather than triggering the process via an environment variable.
Marek, what are your thoughts?

[U-Boot] [PATCH 2/2] usb: hub: remove CONFIG_USB_HUB_MIN_POWER_ON_DELAY

[Marek Vasut]

On Tuesday, June 03, 2014 at 06:38:30 PM, Stephen Warren wrote:

[...]

> Yes, perhaps it is worth U-Boot probing for longer than the minimum
> time, either always or on-demand as requested by an environment
> variable. The only downside would be that "usb start" would take longer
> even in the absence of these broken(?) devices if we always delay
> longer, rather than triggering the process via an environment variable.
> Marek, what are your thoughts?

I'd vouch for env variable, we do not want to inconvenience users because of 
such bugged devices.

Best regards,
Marek Vasut

[U-Boot] [PATCH 2/2] usb: hub: remove CONFIG_USB_HUB_MIN_POWER_ON_DELAY

[Tim Harvey]

On Tue, Jun 3, 2014 at 9:38 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 06/03/2014 10:17 AM, Tim Harvey wrote:
>> On Mon, May 19, 2014 at 1:21 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>> From: Stephen Warren <swarren@nvidia.com>
>>>
>>> Now that we wait the correct specification-mandated time at the end of
>>> usb_hub_power_on(), I suspect that CONFIG_USB_HUB_MIN_POWER_ON_DELAY has
>>> no purpose.
>>>
>>> For cm_t35.h, we already wait longer than the original MIN_POWER_ON_DELAY,
>>> so this change is safe.
>>>
>>> For gw_ventana.h, we will wait as long as the original MIN_POWER_ON_DELAY
>>> iff pgood_delay was at least 200ms. I'm not sure if this is the case or
>>> not, hence I've CC'd relevant people to test this change.
>
>>> diff --git a/include/configs/gw_ventana.h b/include/configs/gw_ventana.h
>
>>> -#define CONFIG_USB_HUB_MIN_POWER_ON_DELAY 1200
>
>> Stephen,
>>
>> Sorry for the late reply - I'm just getting around to testing this
>> (and I realize that Marek already committed it which is ok by me).
>>
>> I have a variety of USB Mass Storage devices that I tested when I was
>> looking at this and out of about 12 devices I found I had 1 'usb
>> stick' that requires 2100ms in order to respond and be successfully
>
> Was that 2100ms or 1200ms? I ask because gw_ventana.h only had 1200, so
> if devices require 2100ms then they presumably didn't work with the code
> before my patch?

it was 2100ms for that particular device. I don't have in my notes why
I chose 1200 over 1000 but a quick test today showed all other USB
sticks I have worked fine with your 1000ms change.

>
>> scanned: 048d:1327 Integrated Technology Express, Inc 32GB USB stick.
>> I also found that rotational media (ie Seagate and Western Digital USB
>> drives) would not respond in 1000ms either which didn't surprise me as
>> I figured they needed some extra spin-up time. For all other devices I
>> had I found that 1000ms was adequate.
>>
>> So do these devices I mention simply violate the USB spec?
>
> I *think* so yes.
>
>> I wonder if the delay should be able to be overridden with an env var
>> or an argument to 'usb start' to account for devices like this?
>
> Yes, perhaps it is worth U-Boot probing for longer than the minimum
> time, either always or on-demand as requested by an environment
> variable. The only downside would be that "usb start" would take longer
> even in the absence of these broken(?) devices if we always delay
> longer, rather than triggering the process via an environment variable.
> Marek, what are your thoughts?

I wouldn't want to hard code a longer default and make everyone else
pay for abnormal devices.

Tim