| Message ID | 1365764680-10917-8-git-send-email-gautam.vivek@samsung.com |
|---|---|
| State | New |
| Headers | show |
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);
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
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
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);
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
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);
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(-)