Patchwork [U-Boot,v3,7/8] usb: hub: Reset only usb 2.0 ports

login
register
mail settings
Submitter Vivek Gautam
Date April 12, 2013, 11:04 a.m.
Message ID <1365764680-10917-8-git-send-email-gautam.vivek@samsung.com>
Download mbox | patch
Permalink /patch/236038/
State Superseded
Delegated to: Marek Vasut
Headers show

Comments

Vivek Gautam - April 12, 2013, 11:04 a.m.
As per XHCI specifications USB 3.0 protocol ports attempt
to advance to 'Enabled' state; however USB 2.0 protocol ports
require software reset to advance them to 'Enabled' state.
Thereby, inferring that software need to reset USB 2.0 protocol
ports invariably (as per EHCI spec or xHCI spec).

Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
---

This patch added in V3(current-version) of this patch-series.

 common/usb_hub.c |   23 +++++++++++++++++++----
 1 files changed, 19 insertions(+), 4 deletions(-)
Julius Werner - April 23, 2013, 5:23 p.m.
Sorry, forgot this one yesterday. I would consider to just drop/revert
this patch entirely. It's not wrong, but it adds complexity where it is
not needed. You don't have to reset SuperSpeed devices, but it shouldn't
hurt either and from what I can tell Linux does it as well.

> As per XHCI specifications USB 3.0 protocol ports attempt
> to advance to 'Enabled' state; however USB 2.0 protocol ports
> require software reset to advance them to 'Enabled' state.
> Thereby, inferring that software need to reset USB 2.0 protocol
> ports invariably (as per EHCI spec or xHCI spec).
> 
> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
> 
> ---
> This patch added in V3(current-version) of this patch-series.
> 
>  common/usb_hub.c |   23 +++++++++++++++++++----
>  1 files changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/common/usb_hub.c b/common/usb_hub.c
> index 1e225e6..eedbcf2 100644
> --- a/common/usb_hub.c
> +++ b/common/usb_hub.c
> @@ -233,6 +233,7 @@ void usb_hub_port_connect_change(struct usb_device *dev, int port)
>  	struct usb_device *usb;
>  	ALLOC_CACHE_ALIGN_BUFFER(struct usb_port_status, portsts, 1);
>  	unsigned short portstatus;
> +	uint32_t do_port_reset = 1;
>  
>  	/* Check status */
>  	if (usb_get_port_status(dev, port + 1, portsts) < 0) {
> @@ -246,6 +247,9 @@ void usb_hub_port_connect_change(struct usb_device *dev, int port)
>  	      le16_to_cpu(portsts->wPortChange),
>  	      portspeed(portstatus));
>  
> +	if (portstatus & (1 << USB_PORT_FEAT_SUPERSPEED))
> +		do_port_reset = 0;
> +
>  	/* Clear the connection change status */
>  	usb_clear_port_feature(dev, port + 1, USB_PORT_FEAT_C_CONNECTION);
>  
> @@ -259,10 +263,21 @@ void usb_hub_port_connect_change(struct usb_device *dev, int port)
>  	}
>  	mdelay(200);
>  
> -	/* Reset the port */
> -	if (hub_port_reset(dev, port, &portstatus) < 0) {
> -		printf("cannot reset port %i!?\n", port + 1);
> -		return;
> +	/*
> +	 * Reset the port:
> +	 * As per xHCI protocol, USB 3.0 devices do not require
> +	 * a port reset, however USB 2.0 device do require the same
> +	 * to let ports proceed to 'enabled' state
> +	 *
> +	 * XXX: Will this break EHCI ??
> +	 * probably not, above condition for 'do_port_reset' checks for
> +	 * speed, and for EHCI it can't reach Super speed anyways.
> +	 */
> +	if (do_port_reset) {
> +		if (hub_port_reset(dev, port, &portstatus) < 0) {
> +			printf("cannot reset port %i!?\n", port + 1);
> +			return;
> +		}
>  	}
>  
>  	mdelay(200);
Marek Vasut - April 24, 2013, 12:21 a.m.
Dear Julius Werner,

> Sorry, forgot this one yesterday. I would consider to just drop/revert
> this patch entirely. It's not wrong, but it adds complexity where it is
> not needed. You don't have to reset SuperSpeed devices, but it shouldn't
> hurt either and from what I can tell Linux does it as well.

Ok, I can drop this one.

> > As per XHCI specifications USB 3.0 protocol ports attempt
> > to advance to 'Enabled' state; however USB 2.0 protocol ports
> > require software reset to advance them to 'Enabled' state.
> > Thereby, inferring that software need to reset USB 2.0 protocol
> > ports invariably (as per EHCI spec or xHCI spec).
> > 
> > Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
> > 
> > ---
> > This patch added in V3(current-version) of this patch-series.
> > 
> >  common/usb_hub.c |   23 +++++++++++++++++++----
> >  1 files changed, 19 insertions(+), 4 deletions(-)
> > 
> > diff --git a/common/usb_hub.c b/common/usb_hub.c
> > index 1e225e6..eedbcf2 100644
> > --- a/common/usb_hub.c
> > +++ b/common/usb_hub.c
> > @@ -233,6 +233,7 @@ void usb_hub_port_connect_change(struct usb_device
> > *dev, int port)
> > 
> >  	struct usb_device *usb;
> >  	ALLOC_CACHE_ALIGN_BUFFER(struct usb_port_status, portsts, 1);
> >  	unsigned short portstatus;
> > 
> > +	uint32_t do_port_reset = 1;
> > 
> >  	/* Check status */
> >  	if (usb_get_port_status(dev, port + 1, portsts) < 0) {
> > 
> > @@ -246,6 +247,9 @@ void usb_hub_port_connect_change(struct usb_device
> > *dev, int port)
> > 
> >  	      le16_to_cpu(portsts->wPortChange),
> >  	      portspeed(portstatus));
> > 
> > +	if (portstatus & (1 << USB_PORT_FEAT_SUPERSPEED))
> > +		do_port_reset = 0;
> > +
> > 
> >  	/* Clear the connection change status */
> >  	usb_clear_port_feature(dev, port + 1, USB_PORT_FEAT_C_CONNECTION);
> > 
> > @@ -259,10 +263,21 @@ void usb_hub_port_connect_change(struct usb_device
> > *dev, int port)
> > 
> >  	}
> >  	mdelay(200);
> > 
> > -	/* Reset the port */
> > -	if (hub_port_reset(dev, port, &portstatus) < 0) {
> > -		printf("cannot reset port %i!?\n", port + 1);
> > -		return;
> > +	/*
> > +	 * Reset the port:
> > +	 * As per xHCI protocol, USB 3.0 devices do not require
> > +	 * a port reset, however USB 2.0 device do require the same
> > +	 * to let ports proceed to 'enabled' state
> > +	 *
> > +	 * XXX: Will this break EHCI ??
> > +	 * probably not, above condition for 'do_port_reset' checks for
> > +	 * speed, and for EHCI it can't reach Super speed anyways.
> > +	 */
> > +	if (do_port_reset) {
> > +		if (hub_port_reset(dev, port, &portstatus) < 0) {
> > +			printf("cannot reset port %i!?\n", port + 1);
> > +			return;
> > +		}
> > 
> >  	}
> >  	
> >  	mdelay(200);

Best regards,
Marek Vasut
Vivek Gautam - April 24, 2013, 6:08 a.m.
On Wed, Apr 24, 2013 at 5:51 AM, Marek Vasut <marex@denx.de> wrote:
> Dear Julius Werner,
>
>> Sorry, forgot this one yesterday. I would consider to just drop/revert
>> this patch entirely. It's not wrong, but it adds complexity where it is
>> not needed. You don't have to reset SuperSpeed devices, but it shouldn't
>> hurt either and from what I can tell Linux does it as well.
>
> Ok, I can drop this one.

Alright, we will drop this.
I think a possible bug related to calling xhci_port_state_to_neutral()
in xhci_submit_root()
(one pointed by Julius in [PATCH v3 3/8] usb: hub: Power-cycle on
root-hub ports) was causing
USB 3.0 protocol ports fail while resetting.
Now things are fine.

>
>> > As per XHCI specifications USB 3.0 protocol ports attempt
>> > to advance to 'Enabled' state; however USB 2.0 protocol ports
>> > require software reset to advance them to 'Enabled' state.
>> > Thereby, inferring that software need to reset USB 2.0 protocol
>> > ports invariably (as per EHCI spec or xHCI spec).
>> >
>> > Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
>> >
>> > ---
>> > This patch added in V3(current-version) of this patch-series.
>> >
>> >  common/usb_hub.c |   23 +++++++++++++++++++----
>> >  1 files changed, 19 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/common/usb_hub.c b/common/usb_hub.c
>> > index 1e225e6..eedbcf2 100644
>> > --- a/common/usb_hub.c
>> > +++ b/common/usb_hub.c
>> > @@ -233,6 +233,7 @@ void usb_hub_port_connect_change(struct usb_device
>> > *dev, int port)
>> >
>> >     struct usb_device *usb;
>> >     ALLOC_CACHE_ALIGN_BUFFER(struct usb_port_status, portsts, 1);
>> >     unsigned short portstatus;
>> >
>> > +   uint32_t do_port_reset = 1;
>> >
>> >     /* Check status */
>> >     if (usb_get_port_status(dev, port + 1, portsts) < 0) {
>> >
>> > @@ -246,6 +247,9 @@ void usb_hub_port_connect_change(struct usb_device
>> > *dev, int port)
>> >
>> >           le16_to_cpu(portsts->wPortChange),
>> >           portspeed(portstatus));
>> >
>> > +   if (portstatus & (1 << USB_PORT_FEAT_SUPERSPEED))
>> > +           do_port_reset = 0;
>> > +
>> >
>> >     /* Clear the connection change status */
>> >     usb_clear_port_feature(dev, port + 1, USB_PORT_FEAT_C_CONNECTION);
>> >
>> > @@ -259,10 +263,21 @@ void usb_hub_port_connect_change(struct usb_device
>> > *dev, int port)
>> >
>> >     }
>> >     mdelay(200);
>> >
>> > -   /* Reset the port */
>> > -   if (hub_port_reset(dev, port, &portstatus) < 0) {
>> > -           printf("cannot reset port %i!?\n", port + 1);
>> > -           return;
>> > +   /*
>> > +    * Reset the port:
>> > +    * As per xHCI protocol, USB 3.0 devices do not require
>> > +    * a port reset, however USB 2.0 device do require the same
>> > +    * to let ports proceed to 'enabled' state
>> > +    *
>> > +    * XXX: Will this break EHCI ??
>> > +    * probably not, above condition for 'do_port_reset' checks for
>> > +    * speed, and for EHCI it can't reach Super speed anyways.
>> > +    */
>> > +   if (do_port_reset) {
>> > +           if (hub_port_reset(dev, port, &portstatus) < 0) {
>> > +                   printf("cannot reset port %i!?\n", port + 1);
>> > +                   return;
>> > +           }
>> >
>> >     }
>> >
>> >     mdelay(200);
>
> Best regards,
> Marek Vasut
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
Vivek Gautam - April 30, 2013, 12:24 p.m.
Hi Marek,


On Wed, Apr 24, 2013 at 11:38 AM, Vivek Gautam
<gautamvivek1987@gmail.com> wrote:
> On Wed, Apr 24, 2013 at 5:51 AM, Marek Vasut <marex@denx.de> wrote:
>> Dear Julius Werner,
>>
>>> Sorry, forgot this one yesterday. I would consider to just drop/revert
>>> this patch entirely. It's not wrong, but it adds complexity where it is
>>> not needed. You don't have to reset SuperSpeed devices, but it shouldn't
>>> hurt either and from what I can tell Linux does it as well.
>>
>> Ok, I can drop this one.

We need to drop this from u-boot-usb/next branch, right ?
I am sure this is added to you to-do list, just wanted to make sure
since i could
see it in 'next' right now.
Thanks !!

>
> Alright, we will drop this.
> I think a possible bug related to calling xhci_port_state_to_neutral()
> in xhci_submit_root()
> (one pointed by Julius in [PATCH v3 3/8] usb: hub: Power-cycle on
> root-hub ports) was causing
> USB 3.0 protocol ports fail while resetting.
> Now things are fine.
>
>>
>>> > As per XHCI specifications USB 3.0 protocol ports attempt
>>> > to advance to 'Enabled' state; however USB 2.0 protocol ports
>>> > require software reset to advance them to 'Enabled' state.
>>> > Thereby, inferring that software need to reset USB 2.0 protocol
>>> > ports invariably (as per EHCI spec or xHCI spec).
>>> >
>>> > Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
>>> >
>>> > ---
>>> > This patch added in V3(current-version) of this patch-series.
>>> >
>>> >  common/usb_hub.c |   23 +++++++++++++++++++----
>>> >  1 files changed, 19 insertions(+), 4 deletions(-)
>>> >
>>> > diff --git a/common/usb_hub.c b/common/usb_hub.c
>>> > index 1e225e6..eedbcf2 100644
>>> > --- a/common/usb_hub.c
>>> > +++ b/common/usb_hub.c
>>> > @@ -233,6 +233,7 @@ void usb_hub_port_connect_change(struct usb_device
>>> > *dev, int port)
>>> >
>>> >     struct usb_device *usb;
>>> >     ALLOC_CACHE_ALIGN_BUFFER(struct usb_port_status, portsts, 1);
>>> >     unsigned short portstatus;
>>> >
>>> > +   uint32_t do_port_reset = 1;
>>> >
>>> >     /* Check status */
>>> >     if (usb_get_port_status(dev, port + 1, portsts) < 0) {
>>> >
>>> > @@ -246,6 +247,9 @@ void usb_hub_port_connect_change(struct usb_device
>>> > *dev, int port)
>>> >
>>> >           le16_to_cpu(portsts->wPortChange),
>>> >           portspeed(portstatus));
>>> >
>>> > +   if (portstatus & (1 << USB_PORT_FEAT_SUPERSPEED))
>>> > +           do_port_reset = 0;
>>> > +
>>> >
>>> >     /* Clear the connection change status */
>>> >     usb_clear_port_feature(dev, port + 1, USB_PORT_FEAT_C_CONNECTION);
>>> >
>>> > @@ -259,10 +263,21 @@ void usb_hub_port_connect_change(struct usb_device
>>> > *dev, int port)
>>> >
>>> >     }
>>> >     mdelay(200);
>>> >
>>> > -   /* Reset the port */
>>> > -   if (hub_port_reset(dev, port, &portstatus) < 0) {
>>> > -           printf("cannot reset port %i!?\n", port + 1);
>>> > -           return;
>>> > +   /*
>>> > +    * Reset the port:
>>> > +    * As per xHCI protocol, USB 3.0 devices do not require
>>> > +    * a port reset, however USB 2.0 device do require the same
>>> > +    * to let ports proceed to 'enabled' state
>>> > +    *
>>> > +    * XXX: Will this break EHCI ??
>>> > +    * probably not, above condition for 'do_port_reset' checks for
>>> > +    * speed, and for EHCI it can't reach Super speed anyways.
>>> > +    */
>>> > +   if (do_port_reset) {
>>> > +           if (hub_port_reset(dev, port, &portstatus) < 0) {
>>> > +                   printf("cannot reset port %i!?\n", port + 1);
>>> > +                   return;
>>> > +           }
>>> >
>>> >     }
>>> >
>>> >     mdelay(200);
Marek Vasut - April 30, 2013, 5:11 p.m.
Dear Vivek Gautam,

> Hi Marek,
> 
> 
> On Wed, Apr 24, 2013 at 11:38 AM, Vivek Gautam
> 
> <gautamvivek1987@gmail.com> wrote:
> > On Wed, Apr 24, 2013 at 5:51 AM, Marek Vasut <marex@denx.de> wrote:
> >> Dear Julius Werner,
> >> 
> >>> Sorry, forgot this one yesterday. I would consider to just drop/revert
> >>> this patch entirely. It's not wrong, but it adds complexity where it is
> >>> not needed. You don't have to reset SuperSpeed devices, but it
> >>> shouldn't hurt either and from what I can tell Linux does it as well.
> >> 
> >> Ok, I can drop this one.
> 
> We need to drop this from u-boot-usb/next branch, right ?
> I am sure this is added to you to-do list, just wanted to make sure
> since i could
> see it in 'next' right now.
> Thanks !!

Dropped and pushed to -next .

Best regards,
Marek Vasut

Patch

diff --git a/common/usb_hub.c b/common/usb_hub.c
index 1e225e6..eedbcf2 100644
--- a/common/usb_hub.c
+++ b/common/usb_hub.c
@@ -233,6 +233,7 @@  void usb_hub_port_connect_change(struct usb_device *dev, int port)
 	struct usb_device *usb;
 	ALLOC_CACHE_ALIGN_BUFFER(struct usb_port_status, portsts, 1);
 	unsigned short portstatus;
+	uint32_t do_port_reset = 1;
 
 	/* Check status */
 	if (usb_get_port_status(dev, port + 1, portsts) < 0) {
@@ -246,6 +247,9 @@  void usb_hub_port_connect_change(struct usb_device *dev, int port)
 	      le16_to_cpu(portsts->wPortChange),
 	      portspeed(portstatus));
 
+	if (portstatus & (1 << USB_PORT_FEAT_SUPERSPEED))
+		do_port_reset = 0;
+
 	/* Clear the connection change status */
 	usb_clear_port_feature(dev, port + 1, USB_PORT_FEAT_C_CONNECTION);
 
@@ -259,10 +263,21 @@  void usb_hub_port_connect_change(struct usb_device *dev, int port)
 	}
 	mdelay(200);
 
-	/* Reset the port */
-	if (hub_port_reset(dev, port, &portstatus) < 0) {
-		printf("cannot reset port %i!?\n", port + 1);
-		return;
+	/*
+	 * Reset the port:
+	 * As per xHCI protocol, USB 3.0 devices do not require
+	 * a port reset, however USB 2.0 device do require the same
+	 * to let ports proceed to 'enabled' state
+	 *
+	 * XXX: Will this break EHCI ??
+	 * probably not, above condition for 'do_port_reset' checks for
+	 * speed, and for EHCI it can't reach Super speed anyways.
+	 */
+	if (do_port_reset) {
+		if (hub_port_reset(dev, port, &portstatus) < 0) {
+			printf("cannot reset port %i!?\n", port + 1);
+			return;
+		}
 	}
 
 	mdelay(200);