[U-Boot] [PATCH 0/7] usb: ss: Some fixes and cleanup for USB super-speed support

[U-Boot] [PATCH 0/7] usb: ss: Some fixes and cleanup for USB super-speed support

[Vivek Gautam]

Based on 'u-boot-usb' master branch.

This patch-series includes majorly some clean-up, few fixes and
then some basic super-speed usb infrastructure addition, to help
put support for XHCI in near future.

Few patches here have been pulled from patch-series sent long back
for XHCI support:
[RFC PATCH 0/2] USB: XHCI: Add xHCI host controller stack driver

We plan to send further the xHCI controller's stack driver in near
future.

Vivek Gautam (7):
  USB: Some cleanup prior to USB 3.0 interface addition
  usb: hub: Conditionally power on usb's root-hub ports
  usb: Update device class in usb device's descriptor
  usb: hub: Fix enumration timeout
  usb: hub: Increase device enumeration timeout for broken drives
  USB: SS: Add support for Super Speed USB interface
  usb: eth: Fix for updated usb interface descriptor structure

 common/cmd_usb.c           |    6 +++-
 common/usb.c               |   34 ++++++++++++++++++---------
 common/usb_hub.c           |   54 +++++++++++++++++++++++++++++++++++--------
 common/usb_storage.c       |   30 +++++++++++++-----------
 drivers/usb/eth/asix.c     |   14 ++++++-----
 drivers/usb/eth/smsc95xx.c |   24 +++++++++---------
 include/usb.h              |   12 +++++++++-
 include/usb_defs.h         |   26 +++++++++++++++++++-
 8 files changed, 142 insertions(+), 58 deletions(-)

-- 
1.7.6.5

[U-Boot] [PATCH 1/7] USB: Some cleanup prior to USB 3.0 interface addition

[Vivek Gautam]

Some cleanup in usb framework, nothing much on feature side.

Signed-off-by: Vikas C Sajjan <vikas.sajjan@samsung.com>
Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
---
 common/usb.c         |   18 ++++++++++--------
 common/usb_storage.c |   30 ++++++++++++++++--------------
 include/usb_defs.h   |    2 +-
 3 files changed, 27 insertions(+), 23 deletions(-)

diff --git a/common/usb.c b/common/usb.c
index 6fc0fc1..40c1547 100644
--- a/common/usb.c
+++ b/common/usb.c
@@ -360,6 +360,7 @@ static int usb_parse_config(struct usb_device *dev,
 	int index, ifno, epno, curr_if_num;
 	int i;
 	u16 ep_wMaxPacketSize;
+	struct usb_interface *if_desc = NULL;
 
 	ifno = -1;
 	epno = -1;
@@ -387,23 +388,24 @@ static int usb_parse_config(struct usb_device *dev,
 			     &buffer[index])->bInterfaceNumber != curr_if_num) {
 				/* this is a new interface, copy new desc */
 				ifno = dev->config.no_of_if;
+				if_desc = &dev->config.if_desc[ifno];
 				dev->config.no_of_if++;
-				memcpy(&dev->config.if_desc[ifno],
-					&buffer[index], buffer[index]);
-				dev->config.if_desc[ifno].no_of_ep = 0;
-				dev->config.if_desc[ifno].num_altsetting = 1;
+				memcpy(if_desc,	&buffer[index], buffer[index]);
+				if_desc->no_of_ep = 0;
+				if_desc->num_altsetting = 1;
 				curr_if_num =
-				     dev->config.if_desc[ifno].desc.bInterfaceNumber;
+				     if_desc->desc.bInterfaceNumber;
 			} else {
 				/* found alternate setting for the interface */
-				dev->config.if_desc[ifno].num_altsetting++;
+				if_desc->num_altsetting++;
 			}
 			break;
 		case USB_DT_ENDPOINT:
 			epno = dev->config.if_desc[ifno].no_of_ep;
+			if_desc = &dev->config.if_desc[ifno];
 			/* found an endpoint */
-			dev->config.if_desc[ifno].no_of_ep++;
-			memcpy(&dev->config.if_desc[ifno].ep_desc[epno],
+			if_desc->no_of_ep++;
+			memcpy(&if_desc->ep_desc[epno],
 				&buffer[index], buffer[index]);
 			ep_wMaxPacketSize = get_unaligned(&dev->config.\
 							if_desc[ifno].\
diff --git a/common/usb_storage.c b/common/usb_storage.c
index fb322b4..475c218 100644
--- a/common/usb_storage.c
+++ b/common/usb_storage.c
@@ -278,10 +278,10 @@ int usb_stor_scan(int mode)
 			     lun++) {
 				usb_dev_desc[usb_max_devs].lun = lun;
 				if (usb_stor_get_info(dev, &usb_stor[start],
-						      &usb_dev_desc[usb_max_devs]) == 1) {
-				usb_max_devs++;
-		}
+				    &usb_dev_desc[usb_max_devs]) == 1)
+					usb_max_devs++;
 			}
+
 		}
 		/* if storage device */
 		if (usb_max_devs == USB_MAX_STOR_DEV) {
@@ -511,7 +511,7 @@ static int usb_stor_BBB_comdat(ccb *srb, struct us_data *us)
 	dir_in = US_DIRECTION(srb->cmd[0]);
 
 #ifdef BBB_COMDAT_TRACE
-	printf("dir %d lun %d cmdlen %d cmd %p datalen %d pdata %p\n",
+	printf("dir %d lun %d cmdlen %d cmd %p datalen %lu pdata %p\n",
 		dir_in, srb->lun, srb->cmdlen, srb->cmd, srb->datalen,
 		srb->pdata);
 	if (srb->cmdlen) {
@@ -1218,6 +1218,7 @@ int usb_storage_probe(struct usb_device *dev, unsigned int ifnum,
 {
 	struct usb_interface *iface;
 	int i;
+	struct usb_endpoint_descriptor *ep_desc;
 	unsigned int flags = 0;
 
 	int protocol = 0;
@@ -1300,24 +1301,25 @@ int usb_storage_probe(struct usb_device *dev, unsigned int ifnum,
 	 * We will ignore any others.
 	 */
 	for (i = 0; i < iface->desc.bNumEndpoints; i++) {
+		ep_desc = &iface->ep_desc[i];
 		/* is it an BULK endpoint? */
-		if ((iface->ep_desc[i].bmAttributes &
+		if ((ep_desc->bmAttributes &
 		     USB_ENDPOINT_XFERTYPE_MASK) == USB_ENDPOINT_XFER_BULK) {
-			if (iface->ep_desc[i].bEndpointAddress & USB_DIR_IN)
-				ss->ep_in = iface->ep_desc[i].bEndpointAddress &
-					USB_ENDPOINT_NUMBER_MASK;
+			if (ep_desc->bEndpointAddress & USB_DIR_IN)
+				ss->ep_in = ep_desc->bEndpointAddress &
+						USB_ENDPOINT_NUMBER_MASK;
 			else
 				ss->ep_out =
-					iface->ep_desc[i].bEndpointAddress &
+					ep_desc->bEndpointAddress &
 					USB_ENDPOINT_NUMBER_MASK;
 		}
 
 		/* is it an interrupt endpoint? */
-		if ((iface->ep_desc[i].bmAttributes &
-		    USB_ENDPOINT_XFERTYPE_MASK) == USB_ENDPOINT_XFER_INT) {
-			ss->ep_int = iface->ep_desc[i].bEndpointAddress &
-				USB_ENDPOINT_NUMBER_MASK;
-			ss->irqinterval = iface->ep_desc[i].bInterval;
+		if ((ep_desc->bmAttributes &
+		     USB_ENDPOINT_XFERTYPE_MASK) == USB_ENDPOINT_XFER_INT) {
+			ss->ep_int = ep_desc->bEndpointAddress &
+						USB_ENDPOINT_NUMBER_MASK;
+			ss->irqinterval = ep_desc->bInterval;
 		}
 	}
 	USB_STOR_PRINTF("Endpoints In %d Out %d Int %d\n",
diff --git a/include/usb_defs.h b/include/usb_defs.h
index 9502544..0c78d9d 100644
--- a/include/usb_defs.h
+++ b/include/usb_defs.h
@@ -234,7 +234,7 @@
 #define HUB_CHAR_OCPM               0x0018
 
 /*
- *Hub Status & Hub Change bit masks
+ * Hub Status & Hub Change bit masks
  */
 #define HUB_STATUS_LOCAL_POWER	0x0001
 #define HUB_STATUS_OVERCURRENT	0x0002
-- 
1.7.6.5

[U-Boot] [PATCH 2/7] usb: hub: Conditionally power on usb's root-hub ports

[Vivek Gautam]

Power on root hubs' ports only when they are not yet powered on.
Its seen with USB 3.0 ports that they are powered on after
a H/W reset, as also reflected in XHCI spec (sec 4.3):
"After a Chip Hardware Reset, HCRST, or commanded to the
PLS = RxDetect state, all Root Hub ports shall be in Disconnected
state, i.e. the port is powered on (PP = 1)"

Signed-off-by: Amar <amarendra.xt@samsung.com>
Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
---
 common/usb_hub.c |   22 +++++++++++++++++++---
 1 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/common/usb_hub.c b/common/usb_hub.c
index b5eeb62..0677004 100644
--- a/common/usb_hub.c
+++ b/common/usb_hub.c
@@ -111,13 +111,29 @@ 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 */
+
+	/* Enable power to ports whose power is not yet on */
 	USB_HUB_PRINTF("enabling power on all ports\n");
 	for (i = 0; i < dev->maxchild; i++) {
-		usb_set_port_feature(dev, i + 1, USB_PORT_FEAT_POWER);
-		USB_HUB_PRINTF("port %d returns %lX\n", i + 1, dev->status);
+		ret = usb_get_port_status(dev, i + 1, portsts);
+		if (ret < 0) {
+			USB_HUB_PRINTF("port %d: get_port_status failed\n",
+					i + 1);
+			return;
+		}
+
+		portstatus = le16_to_cpu(portsts->wPortStatus);
+
+		if (!(portstatus & (USB_PORT_STAT_POWER << 1))) {
+			usb_set_port_feature(dev, i + 1, USB_PORT_FEAT_POWER);
+			USB_HUB_PRINTF("port %d returns %lX\n", i + 1,
+					dev->status);
+		}
 	}
 
 	/* Wait at least 100 msec for power to become stable */
-- 
1.7.6.5

[U-Boot] [PATCH 3/7] usb: Update device class in usb device's descriptor

[Vivek Gautam]

Fetch the device class into usb device's dwcriptors,
so that the host controller's driver can use this info
to differentiate between HUB and DEVICE.

Signed-off-by: Amar <amarendra.xt@samsung.com>
---
 common/usb.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/common/usb.c b/common/usb.c
index 40c1547..39fcedd 100644
--- a/common/usb.c
+++ b/common/usb.c
@@ -888,6 +888,11 @@ int usb_new_device(struct usb_device *dev)
 
 	dev->descriptor.bMaxPacketSize0 = desc->bMaxPacketSize0;
 
+	/* Fetch the device class, driver can use this info
+	 * to differentiate between HUB and DEVICE.
+	 */
+	dev->descriptor.bDeviceClass = desc->bDeviceClass;
+
 	/* find the port number we're at */
 	if (parent) {
 		int j;
-- 
1.7.6.5

[U-Boot] [PATCH 4/7] usb: hub: Fix enumration timeout

[Vivek Gautam]

Patch b6d7852c increases timeout for enumeration, taking
worst case to be 10 sec.
get_timer() api returns timestamp in micro-seconds, which is
what we are checking in the do-while() loop in usb_hub_configure()
(get_timer(start) < CONFIG_SYS_HZ * 10).
This should give us a required check for 10 seconds, and thereby
we don't need to add additional mdelay of 100 microseconds in
each cycle.

Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
CC: Vipin Kumar <vipin.kumar@st.com>
---
 common/usb_hub.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/common/usb_hub.c b/common/usb_hub.c
index 0677004..d77f98d 100644
--- a/common/usb_hub.c
+++ b/common/usb_hub.c
@@ -439,7 +439,6 @@ static int usb_hub_configure(struct usb_device *dev)
 				(portstatus & USB_PORT_STAT_CONNECTION))
 				break;
 
-			mdelay(100);
 		} while (get_timer(start) < CONFIG_SYS_HZ * 10);
 
 		if (ret < 0)
-- 
1.7.6.5

[U-Boot] [PATCH 5/7] usb: hub: Increase device enumeration timeout for broken drives

[Vivek Gautam]

Few broken usb mass storage devices can take some time to set
Current Connect Status (CCS) and Connect Status Change (CSC) in
Port status register after an attach.
So increasing some timeout when both CCS and CSC bits are not set.

Signed-off-by: Amar <amarendra.xt@samsung.com>
Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
---
 common/usb_hub.c |   21 ++++++++++++++++++---
 1 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/common/usb_hub.c b/common/usb_hub.c
index d77f98d..8ba7a0d 100644
--- a/common/usb_hub.c
+++ b/common/usb_hub.c
@@ -416,6 +416,7 @@ static int usb_hub_configure(struct usb_device *dev)
 		unsigned short portstatus, portchange;
 		int ret;
 		ulong start = get_timer(0);
+		ulong ts = 0;
 
 		/*
 		 * Wait for (whichever finishes first)
@@ -436,9 +437,23 @@ static int usb_hub_configure(struct usb_device *dev)
 			portchange = le16_to_cpu(portsts->wPortChange);
 
 			if ((portchange & USB_PORT_STAT_C_CONNECTION) ==
-				(portstatus & USB_PORT_STAT_CONNECTION))
-				break;
-
+			    (portstatus & USB_PORT_STAT_CONNECTION)) {
+				/*
+				 * Few brokern pen drives can take some
+				 * time to even report CSC or CCS state after
+				 * an attach. So giving this time lag so that
+				 * the CCS or CSC state can be changed and
+				 * reflected in Port status register.
+				 * XXX: This approach however has a drawback,
+				 * with this we are going to wait for 1 sec
+				 * even for ports on which nothing is connected.
+				 */
+				if (ts == 0)
+					ts = get_timer(0);
+				if ((portchange & USB_PORT_STAT_C_CONNECTION) ||
+				    (get_timer(ts) > CONFIG_SYS_HZ * 1))
+					break;
+			}
 		} while (get_timer(start) < CONFIG_SYS_HZ * 10);
 
 		if (ret < 0)
-- 
1.7.6.5

[U-Boot] [PATCH 6/7] USB: SS: Add support for Super Speed USB interface

[Vivek Gautam]

This adds usb framework support for super-speed usb, which will
further facilitate to add stack support for xHCI.

Signed-off-by: Vikas C Sajjan <vikas.sajjan@samsung.com>
Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
---
 common/cmd_usb.c     |    6 ++++--
 common/usb.c         |   11 ++++++++---
 common/usb_hub.c     |   10 +++++++---
 common/usb_storage.c |    2 +-
 include/usb.h        |   12 +++++++++++-
 include/usb_defs.h   |   24 +++++++++++++++++++++++-
 6 files changed, 54 insertions(+), 11 deletions(-)

diff --git a/common/cmd_usb.c b/common/cmd_usb.c
index dacdc2d..90c2cf1 100644
--- a/common/cmd_usb.c
+++ b/common/cmd_usb.c
@@ -262,7 +262,7 @@ static void usb_display_config(struct usb_device *dev)
 		ifdesc = &config->if_desc[i];
 		usb_display_if_desc(&ifdesc->desc, dev);
 		for (ii = 0; ii < ifdesc->no_of_ep; ii++) {
-			epdesc = &ifdesc->ep_desc[ii];
+			epdesc = &ifdesc->ep_desc[ii].ep_desc;
 			usb_display_ep_desc(epdesc);
 		}
 	}
@@ -271,7 +271,9 @@ static void usb_display_config(struct usb_device *dev)
 
 static inline char *portspeed(int speed)
 {
-	if (speed == USB_SPEED_HIGH)
+	if (speed == USB_SPEED_SUPER)
+		return "5 Gb/s";
+	else if (speed == USB_SPEED_HIGH)
 		return "480 Mb/s";
 	else if (speed == USB_SPEED_LOW)
 		return "1.5 Mb/s";
diff --git a/common/usb.c b/common/usb.c
index 39fcedd..a5b915e 100644
--- a/common/usb.c
+++ b/common/usb.c
@@ -304,7 +304,7 @@ usb_set_maxpacket_ep(struct usb_device *dev, int if_idx, int ep_idx)
 	struct usb_endpoint_descriptor *ep;
 	u16 ep_wMaxPacketSize;
 
-	ep = &dev->config.if_desc[if_idx].ep_desc[ep_idx];
+	ep = &dev->config.if_desc[if_idx].ep_desc[ep_idx].ep_desc;
 
 	b = ep->bEndpointAddress & USB_ENDPOINT_NUMBER_MASK;
 	ep_wMaxPacketSize = get_unaligned(&ep->wMaxPacketSize);
@@ -410,14 +410,19 @@ static int usb_parse_config(struct usb_device *dev,
 			ep_wMaxPacketSize = get_unaligned(&dev->config.\
 							if_desc[ifno].\
 							ep_desc[epno].\
-							wMaxPacketSize);
+							ep_desc.wMaxPacketSize);
 			put_unaligned(le16_to_cpu(ep_wMaxPacketSize),
 					&dev->config.\
 					if_desc[ifno].\
 					ep_desc[epno].\
-					wMaxPacketSize);
+					ep_desc.wMaxPacketSize);
 			USB_PRINTF("if %d, ep %d\n", ifno, epno);
 			break;
+		case USB_DT_SS_ENDPOINT_COMP:
+			if_desc = &dev->config.if_desc[ifno];
+			memcpy(&(if_desc->ep_desc[epno].ss_ep_comp),
+				&buffer[index], buffer[index]);
+			break;
 		default:
 			if (head->bLength == 0)
 				return 1;
diff --git a/common/usb_hub.c b/common/usb_hub.c
index 8ba7a0d..9acaede 100644
--- a/common/usb_hub.c
+++ b/common/usb_hub.c
@@ -158,7 +158,9 @@ static struct usb_hub_device *usb_hub_allocate(void)
 
 static inline char *portspeed(int portstatus)
 {
-	if (portstatus & (1 << USB_PORT_FEAT_HIGHSPEED))
+	if (portstatus & (1 << USB_PORT_FEAT_SUPERSPEED))
+		return "5 Gb/s";
+	else if (portstatus & (1 << USB_PORT_FEAT_HIGHSPEED))
 		return "480 Mb/s";
 	else if (portstatus & (1 << USB_PORT_FEAT_LOWSPEED))
 		return "1.5 Mb/s";
@@ -262,7 +264,9 @@ void usb_hub_port_connect_change(struct usb_device *dev, int port)
 	/* Allocate a new device struct for it */
 	usb = usb_alloc_new_device(dev->controller);
 
-	if (portstatus & USB_PORT_STAT_HIGH_SPEED)
+	if (portstatus & USB_PORT_STAT_SUPER_SPEED)
+		usb->speed = USB_SPEED_SUPER;
+	else if (portstatus & USB_PORT_STAT_HIGH_SPEED)
 		usb->speed = USB_SPEED_HIGH;
 	else if (portstatus & USB_PORT_STAT_LOW_SPEED)
 		usb->speed = USB_SPEED_LOW;
@@ -525,7 +529,7 @@ int usb_hub_probe(struct usb_device *dev, int ifnum)
 	/* Multiple endpoints? What kind of mutant ninja-hub is this? */
 	if (iface->desc.bNumEndpoints != 1)
 		return 0;
-	ep = &iface->ep_desc[0];
+	ep = &iface->ep_desc[0].ep_desc;
 	/* Output endpoint? Curiousier and curiousier.. */
 	if (!(ep->bEndpointAddress & USB_DIR_IN))
 		return 0;
diff --git a/common/usb_storage.c b/common/usb_storage.c
index 475c218..c48da43 100644
--- a/common/usb_storage.c
+++ b/common/usb_storage.c
@@ -1301,7 +1301,7 @@ int usb_storage_probe(struct usb_device *dev, unsigned int ifnum,
 	 * We will ignore any others.
 	 */
 	for (i = 0; i < iface->desc.bNumEndpoints; i++) {
-		ep_desc = &iface->ep_desc[i];
+		ep_desc = &iface->ep_desc[i].ep_desc;
 		/* is it an BULK endpoint? */
 		if ((ep_desc->bmAttributes &
 		     USB_ENDPOINT_XFERTYPE_MASK) == USB_ENDPOINT_XFER_BULK) {
diff --git a/include/usb.h b/include/usb.h
index d79c865..1d832ca 100644
--- a/include/usb.h
+++ b/include/usb.h
@@ -67,6 +67,16 @@ struct devrequest {
 	unsigned short	length;
 } __attribute__ ((packed));
 
+struct usb_ep_desc {
+	struct usb_endpoint_descriptor		ep_desc;
+	/*
+	 * Super Speed Device will have Super Speed Endpoint
+	 * Companion Descriptor  (section 9.6.7 of usb 3.0 spec)
+	 * Revision 1.0 June 6th 2011
+	 */
+	struct usb_ss_ep_comp_descriptor	ss_ep_comp;
+};
+
 /* Interface */
 struct usb_interface {
 	struct usb_interface_descriptor desc;
@@ -75,7 +85,7 @@ struct usb_interface {
 	unsigned char	num_altsetting;
 	unsigned char	act_altsetting;
 
-	struct usb_endpoint_descriptor ep_desc[USB_MAXENDPOINTS];
+	struct usb_ep_desc ep_desc[USB_MAXENDPOINTS];
 } __attribute__ ((packed));
 
 /* Configuration information.. */
diff --git a/include/usb_defs.h b/include/usb_defs.h
index 0c78d9d..e2aaef3 100644
--- a/include/usb_defs.h
+++ b/include/usb_defs.h
@@ -203,6 +203,8 @@
 #define USB_PORT_FEAT_POWER          8
 #define USB_PORT_FEAT_LOWSPEED       9
 #define USB_PORT_FEAT_HIGHSPEED      10
+#define USB_PORT_FEAT_FULLSPEED      11
+#define USB_PORT_FEAT_SUPERSPEED     12
 #define USB_PORT_FEAT_C_CONNECTION   16
 #define USB_PORT_FEAT_C_ENABLE       17
 #define USB_PORT_FEAT_C_SUSPEND      18
@@ -218,8 +220,20 @@
 #define USB_PORT_STAT_POWER         0x0100
 #define USB_PORT_STAT_LOW_SPEED     0x0200
 #define USB_PORT_STAT_HIGH_SPEED    0x0400	/* support for EHCI */
+#define USB_PORT_STAT_FULL_SPEED    0x0800
+#define USB_PORT_STAT_SUPER_SPEED   0x1000	/* support for XHCI */
 #define USB_PORT_STAT_SPEED	\
-	(USB_PORT_STAT_LOW_SPEED | USB_PORT_STAT_HIGH_SPEED)
+	(USB_PORT_STAT_LOW_SPEED | USB_PORT_STAT_HIGH_SPEED | \
+	USB_PORT_STAT_FULL_SPEED | USB_PORT_STAT_SUPER_SPEED)
+
+/*
+ * Additions to wPortStatus bit field from USB 3.0
+ * See USB 3.0 spec Table 10-10
+ */
+#define USB_PORT_STAT_LINK_STATE	0x01e0
+#define USB_SS_PORT_STAT_POWER		0x0200
+#define USB_SS_PORT_STAT_SPEED		0x1c00
+#define USB_PORT_STAT_SPEED_5GBPS	0x0000
 
 /* wPortChange bits */
 #define USB_PORT_STAT_C_CONNECTION  0x0001
@@ -228,6 +242,14 @@
 #define USB_PORT_STAT_C_OVERCURRENT 0x0008
 #define USB_PORT_STAT_C_RESET       0x0010
 
+/*
+ * Addition to wPortChange bit fields form USB 3.0
+ * See USB 3.0 spec Table 10-11
+ */
+#define USB_PORT_STAT_C_BH_RESET	0x0020
+#define USB_PORT_STAT_C_LINK_STATE	0x0040
+#define USB_PORT_STAT_C_CONFIG_ERROR	0x0080
+
 /* wHubCharacteristics (masks) */
 #define HUB_CHAR_LPSM               0x0003
 #define HUB_CHAR_COMPOUND           0x0004
-- 
1.7.6.5

[U-Boot] [PATCH 7/7] usb: eth: Fix for updated usb interface descriptor structure

[Vivek Gautam]

Now usb interface structure doesn't contain the Ep descriptor directly,
in fact it contains a pointer to 'usb_ep_desc' structure which
contains the Ep descriptor.
So updating the usb-eth drivers accordingly.

Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
---
 drivers/usb/eth/asix.c     |   14 ++++++++------
 drivers/usb/eth/smsc95xx.c |   24 ++++++++++++------------
 2 files changed, 20 insertions(+), 18 deletions(-)

diff --git a/drivers/usb/eth/asix.c b/drivers/usb/eth/asix.c
index 75ec8f7..42e98f1 100644
--- a/drivers/usb/eth/asix.c
+++ b/drivers/usb/eth/asix.c
@@ -611,6 +611,7 @@ int asix_eth_probe(struct usb_device *dev, unsigned int ifnum,
 {
 	struct usb_interface *iface;
 	struct usb_interface_descriptor *iface_desc;
+	struct usb_endpoint_descriptor *ep_desc;
 	int ep_in_found = 0, ep_out_found = 0;
 	int i;
 
@@ -652,10 +653,11 @@ int asix_eth_probe(struct usb_device *dev, unsigned int ifnum,
 	 * int. We will ignore any others.
 	 */
 	for (i = 0; i < iface_desc->bNumEndpoints; i++) {
+		ep_desc = &iface->ep_desc[i].ep_desc;
 		/* is it an BULK endpoint? */
-		if ((iface->ep_desc[i].bmAttributes &
+		if ((ep_desc->bmAttributes &
 		     USB_ENDPOINT_XFERTYPE_MASK) == USB_ENDPOINT_XFER_BULK) {
-			u8 ep_addr = iface->ep_desc[i].bEndpointAddress;
+			u8 ep_addr = ep_desc->bEndpointAddress;
 			if (ep_addr & USB_DIR_IN) {
 				if (!ep_in_found) {
 					ss->ep_in = ep_addr &
@@ -672,11 +674,11 @@ int asix_eth_probe(struct usb_device *dev, unsigned int ifnum,
 		}
 
 		/* is it an interrupt endpoint? */
-		if ((iface->ep_desc[i].bmAttributes &
+		if ((ep_desc->bmAttributes &
 		    USB_ENDPOINT_XFERTYPE_MASK) == USB_ENDPOINT_XFER_INT) {
-			ss->ep_int = iface->ep_desc[i].bEndpointAddress &
-				USB_ENDPOINT_NUMBER_MASK;
-			ss->irqinterval = iface->ep_desc[i].bInterval;
+			ss->ep_int = ep_desc->bEndpointAddress &
+						USB_ENDPOINT_NUMBER_MASK;
+			ss->irqinterval = ep_desc->bInterval;
 		}
 	}
 	debug("Endpoints In %d Out %d Int %d\n",
diff --git a/drivers/usb/eth/smsc95xx.c b/drivers/usb/eth/smsc95xx.c
index fd8f8a7..3f58c89 100644
--- a/drivers/usb/eth/smsc95xx.c
+++ b/drivers/usb/eth/smsc95xx.c
@@ -807,6 +807,7 @@ int smsc95xx_eth_probe(struct usb_device *dev, unsigned int ifnum,
 {
 	struct usb_interface *iface;
 	struct usb_interface_descriptor *iface_desc;
+	struct usb_endpoint_descriptor *ep_desc;
 	int i;
 
 	/* let's examine the device now */
@@ -837,25 +838,24 @@ int smsc95xx_eth_probe(struct usb_device *dev, unsigned int ifnum,
 	 * We will ignore any others.
 	 */
 	for (i = 0; i < iface_desc->bNumEndpoints; i++) {
+		ep_desc = &iface->ep_desc[i].ep_desc;
 		/* is it an BULK endpoint? */
-		if ((iface->ep_desc[i].bmAttributes &
+		if ((ep_desc->bmAttributes &
 		     USB_ENDPOINT_XFERTYPE_MASK) == USB_ENDPOINT_XFER_BULK) {
-			if (iface->ep_desc[i].bEndpointAddress & USB_DIR_IN)
-				ss->ep_in =
-					iface->ep_desc[i].bEndpointAddress &
-					USB_ENDPOINT_NUMBER_MASK;
+			if (ep_desc->bEndpointAddress &	USB_DIR_IN)
+				ss->ep_in = ep_desc->bEndpointAddress &
+						USB_ENDPOINT_NUMBER_MASK;
 			else
-				ss->ep_out =
-					iface->ep_desc[i].bEndpointAddress &
-					USB_ENDPOINT_NUMBER_MASK;
+				ss->ep_out = ep_desc->bEndpointAddress &
+						USB_ENDPOINT_NUMBER_MASK;
 		}
 
 		/* is it an interrupt endpoint? */
-		if ((iface->ep_desc[i].bmAttributes &
+		if ((ep_desc->bmAttributes &
 		    USB_ENDPOINT_XFERTYPE_MASK) == USB_ENDPOINT_XFER_INT) {
-			ss->ep_int = iface->ep_desc[i].bEndpointAddress &
-				USB_ENDPOINT_NUMBER_MASK;
-			ss->irqinterval = iface->ep_desc[i].bInterval;
+			ss->ep_int = ep_desc->bEndpointAddress &
+						USB_ENDPOINT_NUMBER_MASK;
+			ss->irqinterval = ep_desc->bInterval;
 		}
 	}
 	debug("Endpoints In %d Out %d Int %d\n",
-- 
1.7.6.5

[U-Boot] [PATCH 4/7] usb: hub: Fix enumration timeout

[Vipin Kumar]

On 3/27/2013 2:59 PM, Vivek Gautam wrote:
> Patch b6d7852c increases timeout for enumeration, taking
> worst case to be 10 sec.
> get_timer() api returns timestamp in micro-seconds, which is
> what we are checking in the do-while() loop in usb_hub_configure()
> (get_timer(start)<  CONFIG_SYS_HZ * 10).
> This should give us a required check for 10 seconds, and thereby
> we don't need to add additional mdelay of 100 microseconds in
> each cycle.
>
> Signed-off-by: Vivek Gautam<gautam.vivek@samsung.com>
> CC: Vipin Kumar<vipin.kumar@st.com>
> ---
>   common/usb_hub.c |    1 -
>   1 files changed, 0 insertions(+), 1 deletions(-)
>
> diff --git a/common/usb_hub.c b/common/usb_hub.c
> index 0677004..d77f98d 100644
> --- a/common/usb_hub.c
> +++ b/common/usb_hub.c
> @@ -439,7 +439,6 @@ static int usb_hub_configure(struct usb_device *dev)
>   				(portstatus&  USB_PORT_STAT_CONNECTION))
>   				break;
>
> -			mdelay(100);
>   		} while (get_timer(start)<  CONFIG_SYS_HZ * 10);
>
>   		if (ret<  0)

With this change, we are continuously reading the uhb status. Although 
this is also OK, but I feel 100 ms delay is better

Still, there is no harm even if this patch is added. So,

Reviewed-by: Vipin Kumar <vipin.kumar@st.com>

[U-Boot] [PATCH 1/7] USB: Some cleanup prior to USB 3.0 interface addition

[Marek Vasut]

Dear Vivek Gautam,

> Some cleanup in usb framework, nothing much on feature side.
> 
> Signed-off-by: Vikas C Sajjan <vikas.sajjan@samsung.com>
> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
> ---
>  common/usb.c         |   18 ++++++++++--------
>  common/usb_storage.c |   30 ++++++++++++++++--------------
>  include/usb_defs.h   |    2 +-
>  3 files changed, 27 insertions(+), 23 deletions(-)
> 
> diff --git a/common/usb.c b/common/usb.c
> index 6fc0fc1..40c1547 100644
> --- a/common/usb.c
> +++ b/common/usb.c
> @@ -360,6 +360,7 @@ static int usb_parse_config(struct usb_device *dev,
>  	int index, ifno, epno, curr_if_num;
>  	int i;
>  	u16 ep_wMaxPacketSize;
> +	struct usb_interface *if_desc = NULL;
> 
>  	ifno = -1;
>  	epno = -1;
> @@ -387,23 +388,24 @@ static int usb_parse_config(struct usb_device *dev,
>  			     &buffer[index])->bInterfaceNumber != curr_if_num) {
>  				/* this is a new interface, copy new desc */
>  				ifno = dev->config.no_of_if;
> +				if_desc = &dev->config.if_desc[ifno];
>  				dev->config.no_of_if++;
> -				memcpy(&dev->config.if_desc[ifno],
> -					&buffer[index], buffer[index]);
> -				dev->config.if_desc[ifno].no_of_ep = 0;
> -				dev->config.if_desc[ifno].num_altsetting = 1;
> +				memcpy(if_desc,	&buffer[index], buffer[index]);
> +				if_desc->no_of_ep = 0;
> +				if_desc->num_altsetting = 1;
>  				curr_if_num =
> -				     dev-
>config.if_desc[ifno].desc.bInterfaceNumber;
> +				     if_desc->desc.bInterfaceNumber;
>  			} else {
>  				/* found alternate setting for the interface */
> -				dev->config.if_desc[ifno].num_altsetting++;
> +				if_desc->num_altsetting++;

This will crash as if_desc is set in the if() branch above and it will be NULL 
in the else{} branch.

>  			}
>  			break;
>  		case USB_DT_ENDPOINT:
>  			epno = dev->config.if_desc[ifno].no_of_ep;
> +			if_desc = &dev->config.if_desc[ifno];
>  			/* found an endpoint */
> -			dev->config.if_desc[ifno].no_of_ep++;
> -			memcpy(&dev->config.if_desc[ifno].ep_desc[epno],
> +			if_desc->no_of_ep++;
> +			memcpy(&if_desc->ep_desc[epno],
>  				&buffer[index], buffer[index]);
>  			ep_wMaxPacketSize = get_unaligned(&dev->config.\
>  							if_desc[ifno].\
> diff --git a/common/usb_storage.c b/common/usb_storage.c
> index fb322b4..475c218 100644
> --- a/common/usb_storage.c
> +++ b/common/usb_storage.c
> @@ -278,10 +278,10 @@ int usb_stor_scan(int mode)
>  			     lun++) {
>  				usb_dev_desc[usb_max_devs].lun = lun;
>  				if (usb_stor_get_info(dev, &usb_stor[start],
> -						      
&usb_dev_desc[usb_max_devs]) == 1) {
> -				usb_max_devs++;
> -		}
> +				    &usb_dev_desc[usb_max_devs]) == 1)
> +					usb_max_devs++;
>  			}
> +


What is this change here doing?

>  		}
>  		/* if storage device */
>  		if (usb_max_devs == USB_MAX_STOR_DEV) {

[..]

Best regards,
Marek Vasut

[U-Boot] [PATCH 2/7] usb: hub: Conditionally power on usb's root-hub ports

[Marek Vasut]

Dear Vivek Gautam,

> Power on root hubs' ports only when they are not yet powered on.
> Its seen with USB 3.0 ports that they are powered on after
> a H/W reset, as also reflected in XHCI spec (sec 4.3):
> "After a Chip Hardware Reset, HCRST, or commanded to the
> PLS = RxDetect state, all Root Hub ports shall be in Disconnected
> state, i.e. the port is powered on (PP = 1)"
> 
> Signed-off-by: Amar <amarendra.xt@samsung.com>
> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>

Just wondering if we shouldn't power-cycle all the ports instead -- aka. turn 
them off and then turn all of them on again.

Best regards,
Marek Vasut

[U-Boot] [PATCH 3/7] usb: Update device class in usb device's descriptor

[Marek Vasut]

Dear Vivek Gautam,

> Fetch the device class into usb device's dwcriptors,
> so that the host controller's driver can use this info
> to differentiate between HUB and DEVICE.
> 
> Signed-off-by: Amar <amarendra.xt@samsung.com>

Is this a full name? Otherwise this patch is OK.

> ---
>  common/usb.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/common/usb.c b/common/usb.c
> index 40c1547..39fcedd 100644
> --- a/common/usb.c
> +++ b/common/usb.c
> @@ -888,6 +888,11 @@ int usb_new_device(struct usb_device *dev)
> 
>  	dev->descriptor.bMaxPacketSize0 = desc->bMaxPacketSize0;
> 
> +	/* Fetch the device class, driver can use this info
> +	 * to differentiate between HUB and DEVICE.
> +	 */
> +	dev->descriptor.bDeviceClass = desc->bDeviceClass;
> +
>  	/* find the port number we're at */
>  	if (parent) {
>  		int j;

Best regards,
Marek Vasut

[U-Boot] [PATCH 5/7] usb: hub: Increase device enumeration timeout for broken drives

[Marek Vasut]

Dear Vivek Gautam,

> Few broken usb mass storage devices can take some time to set
> Current Connect Status (CCS) and Connect Status Change (CSC) in
> Port status register after an attach.
> So increasing some timeout when both CCS and CSC bits are not set.
> 
> Signed-off-by: Amar <amarendra.xt@samsung.com>
> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>

Can we not postpone checking of these CCS and CSC bits for such broken devices? 
This'd at least allow the "good" devices to be detected quickly and while these 
are handled, this would give some time for the "bad" ones to do their job too.

Best regards,
Marek Vasut

[U-Boot] [PATCH 6/7] USB: SS: Add support for Super Speed USB interface

[Marek Vasut]

Dear Vivek Gautam,

> This adds usb framework support for super-speed usb, which will
> further facilitate to add stack support for xHCI.
> 
> Signed-off-by: Vikas C Sajjan <vikas.sajjan@samsung.com>
> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>

[...]

> --- a/include/usb.h
> +++ b/include/usb.h
> @@ -67,6 +67,16 @@ struct devrequest {
>  	unsigned short	length;
>  } __attribute__ ((packed));
> 
> +struct usb_ep_desc {
> +	struct usb_endpoint_descriptor		ep_desc;
> +	/*
> +	 * Super Speed Device will have Super Speed Endpoint
> +	 * Companion Descriptor  (section 9.6.7 of usb 3.0 spec)
> +	 * Revision 1.0 June 6th 2011
> +	 */
> +	struct usb_ss_ep_comp_descriptor	ss_ep_comp;
> +};
> +
>  /* Interface */
>  struct usb_interface {
>  	struct usb_interface_descriptor desc;
> @@ -75,7 +85,7 @@ struct usb_interface {
>  	unsigned char	num_altsetting;
>  	unsigned char	act_altsetting;
> 
> -	struct usb_endpoint_descriptor ep_desc[USB_MAXENDPOINTS];
> +	struct usb_ep_desc ep_desc[USB_MAXENDPOINTS];

Do we really need this struct usb_ep_desc? Can we not just store the 
usb_ss_ep_comp_descriptor here as well?

>  } __attribute__ ((packed));
> 
>  /* Configuration information.. */
> diff --git a/include/usb_defs.h b/include/usb_defs.h
> index 0c78d9d..e2aaef3 100644
> --- a/include/usb_defs.h
> +++ b/include/usb_defs.h
> @@ -203,6 +203,8 @@
>  #define USB_PORT_FEAT_POWER          8
>  #define USB_PORT_FEAT_LOWSPEED       9
>  #define USB_PORT_FEAT_HIGHSPEED      10
> +#define USB_PORT_FEAT_FULLSPEED      11
> +#define USB_PORT_FEAT_SUPERSPEED     12
>  #define USB_PORT_FEAT_C_CONNECTION   16
>  #define USB_PORT_FEAT_C_ENABLE       17
>  #define USB_PORT_FEAT_C_SUSPEND      18
> @@ -218,8 +220,20 @@
>  #define USB_PORT_STAT_POWER         0x0100
>  #define USB_PORT_STAT_LOW_SPEED     0x0200
>  #define USB_PORT_STAT_HIGH_SPEED    0x0400	/* support for EHCI */
> +#define USB_PORT_STAT_FULL_SPEED    0x0800
> +#define USB_PORT_STAT_SUPER_SPEED   0x1000	/* support for XHCI */
>  #define USB_PORT_STAT_SPEED	\
> -	(USB_PORT_STAT_LOW_SPEED | USB_PORT_STAT_HIGH_SPEED)
> +	(USB_PORT_STAT_LOW_SPEED | USB_PORT_STAT_HIGH_SPEED | \
> +	USB_PORT_STAT_FULL_SPEED | USB_PORT_STAT_SUPER_SPEED)
> +
> +/*
> + * Additions to wPortStatus bit field from USB 3.0
> + * See USB 3.0 spec Table 10-10
> + */
> +#define USB_PORT_STAT_LINK_STATE	0x01e0
> +#define USB_SS_PORT_STAT_POWER		0x0200
> +#define USB_SS_PORT_STAT_SPEED		0x1c00
> +#define USB_PORT_STAT_SPEED_5GBPS	0x0000
> 
>  /* wPortChange bits */
>  #define USB_PORT_STAT_C_CONNECTION  0x0001
> @@ -228,6 +242,14 @@
>  #define USB_PORT_STAT_C_OVERCURRENT 0x0008
>  #define USB_PORT_STAT_C_RESET       0x0010
> 
> +/*
> + * Addition to wPortChange bit fields form USB 3.0
> + * See USB 3.0 spec Table 10-11
> + */
> +#define USB_PORT_STAT_C_BH_RESET	0x0020
> +#define USB_PORT_STAT_C_LINK_STATE	0x0040
> +#define USB_PORT_STAT_C_CONFIG_ERROR	0x0080
> +
>  /* wHubCharacteristics (masks) */
>  #define HUB_CHAR_LPSM               0x0003
>  #define HUB_CHAR_COMPOUND           0x0004

[U-Boot] [PATCH 4/7] usb: hub: Fix enumration timeout

[Tom Rini]

On Wed, Mar 27, 2013 at 02:59:00PM +0530, Vivek Gautam wrote:

> Patch b6d7852c increases timeout for enumeration, taking
> worst case to be 10 sec.
> get_timer() api returns timestamp in micro-seconds, which is
> what we are checking in the do-while() loop in usb_hub_configure()
> (get_timer(start) < CONFIG_SYS_HZ * 10).
> This should give us a required check for 10 seconds, and thereby
> we don't need to add additional mdelay of 100 microseconds in
> each cycle.

The wording here is not correct.  get_timer operates in milliseconds not
microseconds.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130401/ddb88042/attachment.pgp>

[U-Boot] [PATCH 4/7] usb: hub: Fix enumration timeout

[Vivek Gautam]

Hi,


On Tue, Apr 2, 2013 at 1:43 AM, Tom Rini <trini@ti.com> wrote:
> On Wed, Mar 27, 2013 at 02:59:00PM +0530, Vivek Gautam wrote:
>
>> Patch b6d7852c increases timeout for enumeration, taking
>> worst case to be 10 sec.
>> get_timer() api returns timestamp in micro-seconds, which is
>> what we are checking in the do-while() loop in usb_hub_configure()
>> (get_timer(start) < CONFIG_SYS_HZ * 10).
>> This should give us a required check for 10 seconds, and thereby
>> we don't need to add additional mdelay of 100 microseconds in
>> each cycle.
>
> The wording here is not correct.  get_timer operates in milliseconds not
> microseconds.

Oops!! Yes of course, my mistake. Thanks

>
> --
> Tom
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>



-- 
Thanks & Regards
Vivek

[U-Boot] [PATCH 4/7] usb: hub: Fix enumration timeout

[Marek Vasut]

Dear Vivek Gautam,

> Hi,
> 
> On Tue, Apr 2, 2013 at 1:43 AM, Tom Rini <trini@ti.com> wrote:
> > On Wed, Mar 27, 2013 at 02:59:00PM +0530, Vivek Gautam wrote:
> >> Patch b6d7852c increases timeout for enumeration, taking
> >> worst case to be 10 sec.
> >> get_timer() api returns timestamp in micro-seconds, which is
> >> what we are checking in the do-while() loop in usb_hub_configure()
> >> (get_timer(start) < CONFIG_SYS_HZ * 10).
> >> This should give us a required check for 10 seconds, and thereby
> >> we don't need to add additional mdelay of 100 microseconds in
> >> each cycle.
> > 
> > The wording here is not correct.  get_timer operates in milliseconds not
> > microseconds.
> 
> Oops!! Yes of course, my mistake. Thanks

I have the patches queued in usb/next, you can send me a fix and I'll squash it 
with what's already in there.

Best regards,
Marek Vasut

[U-Boot] [PATCH 4/7] usb: hub: Fix enumration timeout

[Vivek Gautam]

Hi Marek,


On Tue, Apr 2, 2013 at 10:42 AM, Marek Vasut <marex@denx.de> wrote:
> Dear Vivek Gautam,
>
>> Hi,
>>
>> On Tue, Apr 2, 2013 at 1:43 AM, Tom Rini <trini@ti.com> wrote:
>> > On Wed, Mar 27, 2013 at 02:59:00PM +0530, Vivek Gautam wrote:
>> >> Patch b6d7852c increases timeout for enumeration, taking
>> >> worst case to be 10 sec.
>> >> get_timer() api returns timestamp in micro-seconds, which is
>> >> what we are checking in the do-while() loop in usb_hub_configure()
>> >> (get_timer(start) < CONFIG_SYS_HZ * 10).
>> >> This should give us a required check for 10 seconds, and thereby
>> >> we don't need to add additional mdelay of 100 microseconds in
>> >> each cycle.
>> >
>> > The wording here is not correct.  get_timer operates in milliseconds not
>> > microseconds.
>>
>> Oops!! Yes of course, my mistake. Thanks
>
> I have the patches queued in usb/next, you can send me a fix and I'll squash it
> with what's already in there.

Sure,  i shall send a fix for this.
Should it be good actually to respin the complete patch-series with
the changes you have suggested ??
The patches i could see in the 'next' are the originally posted ones.

>
> Best regards,
> Marek Vasut



-- 
Thanks & Regards
Vivek

[U-Boot] [PATCH 4/7] usb: hub: Fix enumration timeout

[Vivek Gautam]

On Tue, Apr 2, 2013 at 11:04 AM, Vivek Gautam <gautamvivek1987@gmail.com> wrote:
> Hi Marek,
>
>
> On Tue, Apr 2, 2013 at 10:42 AM, Marek Vasut <marex@denx.de> wrote:
>> Dear Vivek Gautam,
>>
>>> Hi,
>>>
>>> On Tue, Apr 2, 2013 at 1:43 AM, Tom Rini <trini@ti.com> wrote:
>>> > On Wed, Mar 27, 2013 at 02:59:00PM +0530, Vivek Gautam wrote:
>>> >> Patch b6d7852c increases timeout for enumeration, taking
>>> >> worst case to be 10 sec.
>>> >> get_timer() api returns timestamp in micro-seconds, which is
>>> >> what we are checking in the do-while() loop in usb_hub_configure()
>>> >> (get_timer(start) < CONFIG_SYS_HZ * 10).
>>> >> This should give us a required check for 10 seconds, and thereby
>>> >> we don't need to add additional mdelay of 100 microseconds in
>>> >> each cycle.
>>> >
>>> > The wording here is not correct.  get_timer operates in milliseconds not
>>> > microseconds.
>>>
>>> Oops!! Yes of course, my mistake. Thanks
>>
>> I have the patches queued in usb/next, you can send me a fix and I'll squash it
>> with what's already in there.
>
> Sure,  i shall send a fix for this.
> Should it be good actually to respin the complete patch-series with
> the changes you have suggested ??

I was out on weekend, so couldn't update the patch-series.

> The patches i could see in the 'next' are the originally posted ones.
>
>>
>> Best regards,
>> Marek Vasut
>
>
>
> --
> Thanks & Regards
> Vivek



-- 
Thanks & Regards
Vivek

[U-Boot] [PATCH 1/7] USB: Some cleanup prior to USB 3.0 interface addition

[Vivek Gautam]

Hi Marek,


On Thu, Mar 28, 2013 at 7:56 PM, Marek Vasut <marex@denx.de> wrote:
> Dear Vivek Gautam,
>
>> Some cleanup in usb framework, nothing much on feature side.
>>
>> Signed-off-by: Vikas C Sajjan <vikas.sajjan@samsung.com>
>> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
>> ---
>>  common/usb.c         |   18 ++++++++++--------
>>  common/usb_storage.c |   30 ++++++++++++++++--------------
>>  include/usb_defs.h   |    2 +-
>>  3 files changed, 27 insertions(+), 23 deletions(-)
>>
>> diff --git a/common/usb.c b/common/usb.c
>> index 6fc0fc1..40c1547 100644
>> --- a/common/usb.c
>> +++ b/common/usb.c
>> @@ -360,6 +360,7 @@ static int usb_parse_config(struct usb_device *dev,
>>       int index, ifno, epno, curr_if_num;
>>       int i;
>>       u16 ep_wMaxPacketSize;
>> +     struct usb_interface *if_desc = NULL;
>>
>>       ifno = -1;
>>       epno = -1;
>> @@ -387,23 +388,24 @@ static int usb_parse_config(struct usb_device *dev,
>>                            &buffer[index])->bInterfaceNumber != curr_if_num) {
>>                               /* this is a new interface, copy new desc */
>>                               ifno = dev->config.no_of_if;
>> +                             if_desc = &dev->config.if_desc[ifno];
>>                               dev->config.no_of_if++;
>> -                             memcpy(&dev->config.if_desc[ifno],
>> -                                     &buffer[index], buffer[index]);
>> -                             dev->config.if_desc[ifno].no_of_ep = 0;
>> -                             dev->config.if_desc[ifno].num_altsetting = 1;
>> +                             memcpy(if_desc, &buffer[index], buffer[index]);
>> +                             if_desc->no_of_ep = 0;
>> +                             if_desc->num_altsetting = 1;
>>                               curr_if_num =
>> -                                  dev-
>>config.if_desc[ifno].desc.bInterfaceNumber;
>> +                                  if_desc->desc.bInterfaceNumber;
>>                       } else {
>>                               /* found alternate setting for the interface */
>> -                             dev->config.if_desc[ifno].num_altsetting++;
>> +                             if_desc->num_altsetting++;
>
> This will crash as if_desc is set in the if() branch above and it will be NULL
> in the else{} branch.

True, will add here
if_desc = &dev->config.if_desc[ifno];

One more thing actually i could notice, "ifno = -1" in this else section.
Is this what we want it here ?

>
>>                       }
>>                       break;
>>               case USB_DT_ENDPOINT:
>>                       epno = dev->config.if_desc[ifno].no_of_ep;
>> +                     if_desc = &dev->config.if_desc[ifno];
>>                       /* found an endpoint */
>> -                     dev->config.if_desc[ifno].no_of_ep++;
>> -                     memcpy(&dev->config.if_desc[ifno].ep_desc[epno],
>> +                     if_desc->no_of_ep++;
>> +                     memcpy(&if_desc->ep_desc[epno],
>>                               &buffer[index], buffer[index]);
>>                       ep_wMaxPacketSize = get_unaligned(&dev->config.\
>>                                                       if_desc[ifno].\
>> diff --git a/common/usb_storage.c b/common/usb_storage.c
>> index fb322b4..475c218 100644
>> --- a/common/usb_storage.c
>> +++ b/common/usb_storage.c
>> @@ -278,10 +278,10 @@ int usb_stor_scan(int mode)
>>                            lun++) {
>>                               usb_dev_desc[usb_max_devs].lun = lun;
>>                               if (usb_stor_get_info(dev, &usb_stor[start],
>> -
> &usb_dev_desc[usb_max_devs]) == 1) {

Do we really need this brace here? We just have one statement under this if.

>> -                             usb_max_devs++;
>> -             }
>> +                                 &usb_dev_desc[usb_max_devs]) == 1)
>> +                                     usb_max_devs++;
>>                       }
>> +
>
>
> What is this change here doing?

There were these unaligned braces for if part, which we removed.

>
>>               }
>>               /* if storage device */
>>               if (usb_max_devs == USB_MAX_STOR_DEV) {
>
> [..]
>
> Best regards,
> Marek Vasut
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot



-- 
Thanks & Regards
Vivek

[U-Boot] [PATCH 1/7] USB: Some cleanup prior to USB 3.0 interface addition

[Vivek Gautam]

On Tue, Apr 2, 2013 at 11:24 AM, Vivek Gautam <gautamvivek1987@gmail.com> wrote:
> Hi Marek,
>
>
> On Thu, Mar 28, 2013 at 7:56 PM, Marek Vasut <marex@denx.de> wrote:
>> Dear Vivek Gautam,
>>
>>> Some cleanup in usb framework, nothing much on feature side.
>>>
>>> Signed-off-by: Vikas C Sajjan <vikas.sajjan@samsung.com>
>>> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
>>> ---
>>>  common/usb.c         |   18 ++++++++++--------
>>>  common/usb_storage.c |   30 ++++++++++++++++--------------
>>>  include/usb_defs.h   |    2 +-
>>>  3 files changed, 27 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/common/usb.c b/common/usb.c
>>> index 6fc0fc1..40c1547 100644
>>> --- a/common/usb.c
>>> +++ b/common/usb.c
>>> @@ -360,6 +360,7 @@ static int usb_parse_config(struct usb_device *dev,
>>>       int index, ifno, epno, curr_if_num;
>>>       int i;
>>>       u16 ep_wMaxPacketSize;
>>> +     struct usb_interface *if_desc = NULL;
>>>
>>>       ifno = -1;
>>>       epno = -1;
>>> @@ -387,23 +388,24 @@ static int usb_parse_config(struct usb_device *dev,
>>>                            &buffer[index])->bInterfaceNumber != curr_if_num) {
>>>                               /* this is a new interface, copy new desc */
>>>                               ifno = dev->config.no_of_if;
>>> +                             if_desc = &dev->config.if_desc[ifno];
>>>                               dev->config.no_of_if++;
>>> -                             memcpy(&dev->config.if_desc[ifno],
>>> -                                     &buffer[index], buffer[index]);
>>> -                             dev->config.if_desc[ifno].no_of_ep = 0;
>>> -                             dev->config.if_desc[ifno].num_altsetting = 1;
>>> +                             memcpy(if_desc, &buffer[index], buffer[index]);
>>> +                             if_desc->no_of_ep = 0;
>>> +                             if_desc->num_altsetting = 1;
>>>                               curr_if_num =
>>> -                                  dev-
>>>config.if_desc[ifno].desc.bInterfaceNumber;
>>> +                                  if_desc->desc.bInterfaceNumber;
>>>                       } else {
>>>                               /* found alternate setting for the interface */
>>> -                             dev->config.if_desc[ifno].num_altsetting++;
>>> +                             if_desc->num_altsetting++;
>>
>> This will crash as if_desc is set in the if() branch above and it will be NULL
>> in the else{} branch.
>
> True, will add here
> if_desc = &dev->config.if_desc[ifno];
>
> One more thing actually i could notice, "ifno = -1" in this else section.
> Is this what we want it here ?
>
>>
>>>                       }
>>>                       break;
>>>               case USB_DT_ENDPOINT:
>>>                       epno = dev->config.if_desc[ifno].no_of_ep;
>>> +                     if_desc = &dev->config.if_desc[ifno];
>>>                       /* found an endpoint */
>>> -                     dev->config.if_desc[ifno].no_of_ep++;
>>> -                     memcpy(&dev->config.if_desc[ifno].ep_desc[epno],
>>> +                     if_desc->no_of_ep++;
>>> +                     memcpy(&if_desc->ep_desc[epno],
>>>                               &buffer[index], buffer[index]);
>>>                       ep_wMaxPacketSize = get_unaligned(&dev->config.\
>>>                                                       if_desc[ifno].\
>>> diff --git a/common/usb_storage.c b/common/usb_storage.c
>>> index fb322b4..475c218 100644
>>> --- a/common/usb_storage.c
>>> +++ b/common/usb_storage.c
>>> @@ -278,10 +278,10 @@ int usb_stor_scan(int mode)
>>>                            lun++) {
>>>                               usb_dev_desc[usb_max_devs].lun = lun;
>>>                               if (usb_stor_get_info(dev, &usb_stor[start],
>>> -
>> &usb_dev_desc[usb_max_devs]) == 1) {
>
> Do we really need this brace here? We just have one statement under this if.
>
>>> -                             usb_max_devs++;
>>> -             }
>>> +                                 &usb_dev_desc[usb_max_devs]) == 1)
>>> +                                     usb_max_devs++;
>>>                       }
>>> +
>>
>>
>> What is this change here doing?
>
> There were these unaligned braces for if part, which we removed.

Please ignore above comment. It was meant for change prior to this, above.
Will remove this extra line here.

>
>>
>>>               }
>>>               /* if storage device */
>>>               if (usb_max_devs == USB_MAX_STOR_DEV) {
>>
>> [..]
>>
>> Best regards,
>> Marek Vasut
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot at lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot
>
>
>
> --
> Thanks & Regards
> Vivek



-- 
Thanks & Regards
Vivek

[U-Boot] [PATCH 1/7] USB: Some cleanup prior to USB 3.0 interface addition

[Marek Vasut]

Dear Vivek Gautam,

> Hi Marek,
> 
> On Thu, Mar 28, 2013 at 7:56 PM, Marek Vasut <marex@denx.de> wrote:
> > Dear Vivek Gautam,
> > 
> >> Some cleanup in usb framework, nothing much on feature side.
> >> 
> >> Signed-off-by: Vikas C Sajjan <vikas.sajjan@samsung.com>
> >> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
> >> ---
> >> 
> >>  common/usb.c         |   18 ++++++++++--------
> >>  common/usb_storage.c |   30 ++++++++++++++++--------------
> >>  include/usb_defs.h   |    2 +-
> >>  3 files changed, 27 insertions(+), 23 deletions(-)
> >> 
> >> diff --git a/common/usb.c b/common/usb.c
> >> index 6fc0fc1..40c1547 100644
> >> --- a/common/usb.c
> >> +++ b/common/usb.c
> >> @@ -360,6 +360,7 @@ static int usb_parse_config(struct usb_device *dev,
> >> 
> >>       int index, ifno, epno, curr_if_num;
> >>       int i;
> >>       u16 ep_wMaxPacketSize;
> >> 
> >> +     struct usb_interface *if_desc = NULL;
> >> 
> >>       ifno = -1;
> >>       epno = -1;
> >> 
> >> @@ -387,23 +388,24 @@ static int usb_parse_config(struct usb_device
> >> *dev,
> >> 
> >>                            &buffer[index])->bInterfaceNumber !=
> >>                            curr_if_num) {
> >>                            
> >>                               /* this is a new interface, copy new desc
> >>                               */ ifno = dev->config.no_of_if;
> >> 
> >> +                             if_desc = &dev->config.if_desc[ifno];
> >> 
> >>                               dev->config.no_of_if++;
> >> 
> >> -                             memcpy(&dev->config.if_desc[ifno],
> >> -                                     &buffer[index], buffer[index]);
> >> -                             dev->config.if_desc[ifno].no_of_ep = 0;
> >> -                             dev->config.if_desc[ifno].num_altsetting =
> >> 1; +                             memcpy(if_desc, &buffer[index],
> >> buffer[index]); +                             if_desc->no_of_ep = 0;
> >> +                             if_desc->num_altsetting = 1;
> >> 
> >>                               curr_if_num =
> >> 
> >> -                                  dev-
> >>
> >>config.if_desc[ifno].desc.bInterfaceNumber;
> >>
> >> +                                  if_desc->desc.bInterfaceNumber;
> >> 
> >>                       } else {
> >>                       
> >>                               /* found alternate setting for the
> >>                               interface */
> >> 
> >> -                            
> >> dev->config.if_desc[ifno].num_altsetting++; +                          
> >>   if_desc->num_altsetting++;
> > 
> > This will crash as if_desc is set in the if() branch above and it will be
> > NULL in the else{} branch.
> 
> True, will add here
> if_desc = &dev->config.if_desc[ifno];

Yea ;-)

> One more thing actually i could notice, "ifno = -1" in this else section.
> Is this what we want it here ?

Maybe I don't see it ?

> >>                       }
> >>                       break;
> >>               
> >>               case USB_DT_ENDPOINT:
> >>                       epno = dev->config.if_desc[ifno].no_of_ep;
> >> 
> >> +                     if_desc = &dev->config.if_desc[ifno];
> >> 
> >>                       /* found an endpoint */
> >> 
> >> -                     dev->config.if_desc[ifno].no_of_ep++;
> >> -                     memcpy(&dev->config.if_desc[ifno].ep_desc[epno],
> >> +                     if_desc->no_of_ep++;
> >> +                     memcpy(&if_desc->ep_desc[epno],
> >> 
> >>                               &buffer[index], buffer[index]);
> >>                       
> >>                       ep_wMaxPacketSize = get_unaligned(&dev->config.\
> >>                       
> >>                                                       if_desc[ifno].\
> >> 
> >> diff --git a/common/usb_storage.c b/common/usb_storage.c
> >> index fb322b4..475c218 100644
> >> --- a/common/usb_storage.c
> >> +++ b/common/usb_storage.c
> >> @@ -278,10 +278,10 @@ int usb_stor_scan(int mode)
> >> 
> >>                            lun++) {
> >>                            
> >>                               usb_dev_desc[usb_max_devs].lun = lun;
> >>                               if (usb_stor_get_info(dev,
> >>                               &usb_stor[start],
> >> 
> >> -
> > 
> > &usb_dev_desc[usb_max_devs]) == 1) {
> 
> Do we really need this brace here? We just have one statement under this
> if.

If the condition is multi-line, then we use brace, yes.

> >> -                             usb_max_devs++;
> >> -             }
> >> +                                 &usb_dev_desc[usb_max_devs]) == 1)
> >> +                                     usb_max_devs++;
> >> 
> >>                       }
> >> 
> >> +
> > 
> > What is this change here doing?
> 
> There were these unaligned braces for if part, which we removed.
> 
> >>               }
> >>               /* if storage device */
> >>               if (usb_max_devs == USB_MAX_STOR_DEV) {
> > 
> > [..]
> > 
> > Best regards,
> > Marek Vasut
> > _______________________________________________
> > U-Boot mailing list
> > U-Boot at lists.denx.de
> > http://lists.denx.de/mailman/listinfo/u-boot

Best regards,
Marek Vasut

[U-Boot] [PATCH 1/7] USB: Some cleanup prior to USB 3.0 interface addition

[Vivek Gautam]

On Tue, Apr 2, 2013 at 11:56 AM, Marek Vasut <marex@denx.de> wrote:
> Dear Vivek Gautam,
>
>> Hi Marek,
>>
>> On Thu, Mar 28, 2013 at 7:56 PM, Marek Vasut <marex@denx.de> wrote:
>> > Dear Vivek Gautam,
>> >
>> >> Some cleanup in usb framework, nothing much on feature side.
>> >>
>> >> Signed-off-by: Vikas C Sajjan <vikas.sajjan@samsung.com>
>> >> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
>> >> ---
>> >>
>> >>  common/usb.c         |   18 ++++++++++--------
>> >>  common/usb_storage.c |   30 ++++++++++++++++--------------
>> >>  include/usb_defs.h   |    2 +-
>> >>  3 files changed, 27 insertions(+), 23 deletions(-)
>> >>
>> >> diff --git a/common/usb.c b/common/usb.c
>> >> index 6fc0fc1..40c1547 100644
>> >> --- a/common/usb.c
>> >> +++ b/common/usb.c
>> >> @@ -360,6 +360,7 @@ static int usb_parse_config(struct usb_device *dev,
>> >>
>> >>       int index, ifno, epno, curr_if_num;
>> >>       int i;
>> >>       u16 ep_wMaxPacketSize;
>> >>
>> >> +     struct usb_interface *if_desc = NULL;
>> >>
>> >>       ifno = -1;
>> >>       epno = -1;
>> >>
>> >> @@ -387,23 +388,24 @@ static int usb_parse_config(struct usb_device
>> >> *dev,
>> >>
>> >>                            &buffer[index])->bInterfaceNumber !=
>> >>                            curr_if_num) {
>> >>
>> >>                               /* this is a new interface, copy new desc
>> >>                               */ ifno = dev->config.no_of_if;
>> >>
>> >> +                             if_desc = &dev->config.if_desc[ifno];
>> >>
>> >>                               dev->config.no_of_if++;
>> >>
>> >> -                             memcpy(&dev->config.if_desc[ifno],
>> >> -                                     &buffer[index], buffer[index]);
>> >> -                             dev->config.if_desc[ifno].no_of_ep = 0;
>> >> -                             dev->config.if_desc[ifno].num_altsetting =
>> >> 1; +                             memcpy(if_desc, &buffer[index],
>> >> buffer[index]); +                             if_desc->no_of_ep = 0;
>> >> +                             if_desc->num_altsetting = 1;
>> >>
>> >>                               curr_if_num =
>> >>
>> >> -                                  dev-
>> >>
>> >>config.if_desc[ifno].desc.bInterfaceNumber;
>> >>
>> >> +                                  if_desc->desc.bInterfaceNumber;
>> >>
>> >>                       } else {
>> >>
>> >>                               /* found alternate setting for the
>> >>                               interface */
>> >>
>> >> -
>> >> dev->config.if_desc[ifno].num_altsetting++; +
>> >>   if_desc->num_altsetting++;
>> >
>> > This will crash as if_desc is set in the if() branch above and it will be
>> > NULL in the else{} branch.
>>
>> True, will add here
>> if_desc = &dev->config.if_desc[ifno];
>
> Yea ;-)
>
>> One more thing actually i could notice, "ifno = -1" in this else section.
>> Is this what we want it here ?
>
> Maybe I don't see it ?

ifno is initailized to -1.
Now when descriptor is 'Interface type' (case USB_DT_INTERFACE) ifno
is update in if part only as below:
ifno = dev->config.no_of_if;

This would not be reaching else part, right ?

>
>> >>                       }
>> >>                       break;
>> >>
>> >>               case USB_DT_ENDPOINT:
>> >>                       epno = dev->config.if_desc[ifno].no_of_ep;
>> >>
>> >> +                     if_desc = &dev->config.if_desc[ifno];
>> >>
>> >>                       /* found an endpoint */
>> >>
>> >> -                     dev->config.if_desc[ifno].no_of_ep++;
>> >> -                     memcpy(&dev->config.if_desc[ifno].ep_desc[epno],
>> >> +                     if_desc->no_of_ep++;
>> >> +                     memcpy(&if_desc->ep_desc[epno],
>> >>
>> >>                               &buffer[index], buffer[index]);
>> >>
>> >>                       ep_wMaxPacketSize = get_unaligned(&dev->config.\
>> >>
>> >>                                                       if_desc[ifno].\
>> >>
>> >> diff --git a/common/usb_storage.c b/common/usb_storage.c
>> >> index fb322b4..475c218 100644
>> >> --- a/common/usb_storage.c
>> >> +++ b/common/usb_storage.c
>> >> @@ -278,10 +278,10 @@ int usb_stor_scan(int mode)
>> >>
>> >>                            lun++) {
>> >>
>> >>                               usb_dev_desc[usb_max_devs].lun = lun;
>> >>                               if (usb_stor_get_info(dev,
>> >>                               &usb_stor[start],
>> >>
>> >> -
>> >
>> > &usb_dev_desc[usb_max_devs]) == 1) {
>>
>> Do we really need this brace here? We just have one statement under this
>> if.
>
> If the condition is multi-line, then we use brace, yes.

Alright, will amend this. :-)

>
>> >> -                             usb_max_devs++;
>> >> -             }
>> >> +                                 &usb_dev_desc[usb_max_devs]) == 1)
>> >> +                                     usb_max_devs++;
>> >>
>> >>                       }
>> >>
>> >> +
>> >
>> > What is this change here doing?
>>
>> There were these unaligned braces for if part, which we removed.
>>
>> >>               }
>> >>               /* if storage device */
>> >>               if (usb_max_devs == USB_MAX_STOR_DEV) {
[..]



-- 
Thanks & Regards
Vivek

[U-Boot] [PATCH 4/7] usb: hub: Fix enumration timeout

[Marek Vasut]

Dear Vivek Gautam,

> On Tue, Apr 2, 2013 at 11:04 AM, Vivek Gautam <gautamvivek1987@gmail.com> 
wrote:
> > Hi Marek,
> > 
> > On Tue, Apr 2, 2013 at 10:42 AM, Marek Vasut <marex@denx.de> wrote:
> >> Dear Vivek Gautam,
> >> 
> >>> Hi,
> >>> 
> >>> On Tue, Apr 2, 2013 at 1:43 AM, Tom Rini <trini@ti.com> wrote:
> >>> > On Wed, Mar 27, 2013 at 02:59:00PM +0530, Vivek Gautam wrote:
> >>> >> Patch b6d7852c increases timeout for enumeration, taking
> >>> >> worst case to be 10 sec.
> >>> >> get_timer() api returns timestamp in micro-seconds, which is
> >>> >> what we are checking in the do-while() loop in usb_hub_configure()
> >>> >> (get_timer(start) < CONFIG_SYS_HZ * 10).
> >>> >> This should give us a required check for 10 seconds, and thereby
> >>> >> we don't need to add additional mdelay of 100 microseconds in
> >>> >> each cycle.
> >>> > 
> >>> > The wording here is not correct.  get_timer operates in milliseconds
> >>> > not microseconds.
> >>> 
> >>> Oops!! Yes of course, my mistake. Thanks
> >> 
> >> I have the patches queued in usb/next, you can send me a fix and I'll
> >> squash it with what's already in there.
> > 
> > Sure,  i shall send a fix for this.
> > Should it be good actually to respin the complete patch-series with
> > the changes you have suggested ??
> 
> I was out on weekend, so couldn't update the patch-series.

No problem, I'll drop it from next then, please send the whole series.

Thanks!

Best regards,
Marek Vasut

[U-Boot] [PATCH 1/7] USB: Some cleanup prior to USB 3.0 interface addition

[Marek Vasut]

Dear Vivek Gautam,

> On Tue, Apr 2, 2013 at 11:56 AM, Marek Vasut <marex@denx.de> wrote:
> > Dear Vivek Gautam,
> > 
> >> Hi Marek,
> >> 
> >> On Thu, Mar 28, 2013 at 7:56 PM, Marek Vasut <marex@denx.de> wrote:
> >> > Dear Vivek Gautam,
> >> > 
> >> >> Some cleanup in usb framework, nothing much on feature side.
> >> >> 
> >> >> Signed-off-by: Vikas C Sajjan <vikas.sajjan@samsung.com>
> >> >> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
> >> >> ---
> >> >> 
> >> >>  common/usb.c         |   18 ++++++++++--------
> >> >>  common/usb_storage.c |   30 ++++++++++++++++--------------
> >> >>  include/usb_defs.h   |    2 +-
> >> >>  3 files changed, 27 insertions(+), 23 deletions(-)
> >> >> 
> >> >> diff --git a/common/usb.c b/common/usb.c
> >> >> index 6fc0fc1..40c1547 100644
> >> >> --- a/common/usb.c
> >> >> +++ b/common/usb.c
> >> >> @@ -360,6 +360,7 @@ static int usb_parse_config(struct usb_device
> >> >> *dev,
> >> >> 
> >> >>       int index, ifno, epno, curr_if_num;
> >> >>       int i;
> >> >>       u16 ep_wMaxPacketSize;
> >> >> 
> >> >> +     struct usb_interface *if_desc = NULL;
> >> >> 
> >> >>       ifno = -1;
> >> >>       epno = -1;
> >> >> 
> >> >> @@ -387,23 +388,24 @@ static int usb_parse_config(struct usb_device
> >> >> *dev,
> >> >> 
> >> >>                            &buffer[index])->bInterfaceNumber !=
> >> >>                            curr_if_num) {
> >> >>                            
> >> >>                               /* this is a new interface, copy new
> >> >>                               desc */ ifno = dev->config.no_of_if;
> >> >> 
> >> >> +                             if_desc = &dev->config.if_desc[ifno];
> >> >> 
> >> >>                               dev->config.no_of_if++;
> >> >> 
> >> >> -                             memcpy(&dev->config.if_desc[ifno],
> >> >> -                                     &buffer[index], buffer[index]);
> >> >> -                             dev->config.if_desc[ifno].no_of_ep = 0;
> >> >> -                            
> >> >> dev->config.if_desc[ifno].num_altsetting = 1; +                     
> >> >>        memcpy(if_desc, &buffer[index], buffer[index]); +            
> >> >>                 if_desc->no_of_ep = 0; +                            
> >> >> if_desc->num_altsetting = 1;
> >> >> 
> >> >>                               curr_if_num =
> >> >> 
> >> >> -                                  dev-
> >> >>
> >> >>config.if_desc[ifno].desc.bInterfaceNumber;
> >> >>
> >> >> +                                  if_desc->desc.bInterfaceNumber;
> >> >> 
> >> >>                       } else {
> >> >>                       
> >> >>                               /* found alternate setting for the
> >> >>                               interface */
> >> >> 
> >> >> -
> >> >> dev->config.if_desc[ifno].num_altsetting++; +
> >> >> 
> >> >>   if_desc->num_altsetting++;
> >> > 
> >> > This will crash as if_desc is set in the if() branch above and it will
> >> > be NULL in the else{} branch.
> >> 
> >> True, will add here
> >> if_desc = &dev->config.if_desc[ifno];
> > 
> > Yea ;-)
> > 
> >> One more thing actually i could notice, "ifno = -1" in this else
> >> section. Is this what we want it here ?
> > 
> > Maybe I don't see it ?
> 
> ifno is initailized to -1.
> Now when descriptor is 'Interface type' (case USB_DT_INTERFACE) ifno
> is update in if part only as below:
> ifno = dev->config.no_of_if;
> 
> This would not be reaching else part, right ?

Ah I see, yep, indexing the array with -1 is no good. You can add a safety-check 
there , but the -1 should never reach that place indeed.

Best regards,
Marek Vasut

[U-Boot] [PATCH 1/7] USB: Some cleanup prior to USB 3.0 interface addition

[Vivek Gautam]

Hi,


On Tue, Apr 2, 2013 at 12:16 PM, Marek Vasut <marex@denx.de> wrote:
> Dear Vivek Gautam,
>
>> On Tue, Apr 2, 2013 at 11:56 AM, Marek Vasut <marex@denx.de> wrote:
>> > Dear Vivek Gautam,
>> >
>> >> Hi Marek,
>> >>
>> >> On Thu, Mar 28, 2013 at 7:56 PM, Marek Vasut <marex@denx.de> wrote:
>> >> > Dear Vivek Gautam,
>> >> >
>> >> >> Some cleanup in usb framework, nothing much on feature side.
>> >> >>
>> >> >> Signed-off-by: Vikas C Sajjan <vikas.sajjan@samsung.com>
>> >> >> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
>> >> >> ---
>> >> >>
>> >> >>  common/usb.c         |   18 ++++++++++--------
>> >> >>  common/usb_storage.c |   30 ++++++++++++++++--------------
>> >> >>  include/usb_defs.h   |    2 +-
>> >> >>  3 files changed, 27 insertions(+), 23 deletions(-)
>> >> >>
>> >> >> diff --git a/common/usb.c b/common/usb.c
>> >> >> index 6fc0fc1..40c1547 100644
>> >> >> --- a/common/usb.c
>> >> >> +++ b/common/usb.c
>> >> >> @@ -360,6 +360,7 @@ static int usb_parse_config(struct usb_device
>> >> >> *dev,
>> >> >>
>> >> >>       int index, ifno, epno, curr_if_num;
>> >> >>       int i;
>> >> >>       u16 ep_wMaxPacketSize;
>> >> >>
>> >> >> +     struct usb_interface *if_desc = NULL;
>> >> >>
>> >> >>       ifno = -1;
>> >> >>       epno = -1;
>> >> >>
>> >> >> @@ -387,23 +388,24 @@ static int usb_parse_config(struct usb_device
>> >> >> *dev,
>> >> >>
>> >> >>                            &buffer[index])->bInterfaceNumber !=
>> >> >>                            curr_if_num) {
>> >> >>
>> >> >>                               /* this is a new interface, copy new
>> >> >>                               desc */ ifno = dev->config.no_of_if;
>> >> >>
>> >> >> +                             if_desc = &dev->config.if_desc[ifno];
>> >> >>
>> >> >>                               dev->config.no_of_if++;
>> >> >>
>> >> >> -                             memcpy(&dev->config.if_desc[ifno],
>> >> >> -                                     &buffer[index], buffer[index]);
>> >> >> -                             dev->config.if_desc[ifno].no_of_ep = 0;
>> >> >> -
>> >> >> dev->config.if_desc[ifno].num_altsetting = 1; +
>> >> >>        memcpy(if_desc, &buffer[index], buffer[index]); +
>> >> >>                 if_desc->no_of_ep = 0; +
>> >> >> if_desc->num_altsetting = 1;
>> >> >>
>> >> >>                               curr_if_num =
>> >> >>
>> >> >> -                                  dev-
>> >> >>
>> >> >>config.if_desc[ifno].desc.bInterfaceNumber;
>> >> >>
>> >> >> +                                  if_desc->desc.bInterfaceNumber;
>> >> >>
>> >> >>                       } else {
>> >> >>
>> >> >>                               /* found alternate setting for the
>> >> >>                               interface */
>> >> >>
>> >> >> -
>> >> >> dev->config.if_desc[ifno].num_altsetting++; +
>> >> >>
>> >> >>   if_desc->num_altsetting++;
>> >> >
>> >> > This will crash as if_desc is set in the if() branch above and it will
>> >> > be NULL in the else{} branch.
>> >>
>> >> True, will add here
>> >> if_desc = &dev->config.if_desc[ifno];
>> >
>> > Yea ;-)
>> >
>> >> One more thing actually i could notice, "ifno = -1" in this else
>> >> section. Is this what we want it here ?
>> >
>> > Maybe I don't see it ?
>>
>> ifno is initailized to -1.
>> Now when descriptor is 'Interface type' (case USB_DT_INTERFACE) ifno
>> is update in if part only as below:
>> ifno = dev->config.no_of_if;
>>
>> This would not be reaching else part, right ?
>
> Ah I see, yep, indexing the array with -1 is no good. You can add a safety-check
> there , but the -1 should never reach that place indeed.
>

Ok, will amend this as required and update.


-- 
Thanks & Regards
Vivek

[U-Boot] [PATCH 3/7] usb: Update device class in usb device's descriptor

[Vivek Gautam]

CC: Amarendra


On Thu, Mar 28, 2013 at 7:58 PM, Marek Vasut <marex@denx.de> wrote:
> Dear Vivek Gautam,
>
>> Fetch the device class into usb device's dwcriptors,
>> so that the host controller's driver can use this info
>> to differentiate between HUB and DEVICE.
>>
>> Signed-off-by: Amar <amarendra.xt@samsung.com>
>
> Is this a full name? Otherwise this patch is OK.
>
>> ---
>>  common/usb.c |    5 +++++
>>  1 files changed, 5 insertions(+), 0 deletions(-)
>>
>> diff --git a/common/usb.c b/common/usb.c
>> index 40c1547..39fcedd 100644
>> --- a/common/usb.c
>> +++ b/common/usb.c
>> @@ -888,6 +888,11 @@ int usb_new_device(struct usb_device *dev)
>>
>>       dev->descriptor.bMaxPacketSize0 = desc->bMaxPacketSize0;
>>
>> +     /* Fetch the device class, driver can use this info
>> +      * to differentiate between HUB and DEVICE.
>> +      */
>> +     dev->descriptor.bDeviceClass = desc->bDeviceClass;
>> +
>>       /* find the port number we're at */
>>       if (parent) {
>>               int j;
>
> Best regards,
> Marek Vasut
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot



-- 
Thanks & Regards
Vivek

[U-Boot] [PATCH 5/7] usb: hub: Increase device enumeration timeout for broken drives

[Vivek Gautam]

CC: Amarendra


On Thu, Mar 28, 2013 at 8:02 PM, Marek Vasut <marex@denx.de> wrote:
> Dear Vivek Gautam,
>
>> Few broken usb mass storage devices can take some time to set
>> Current Connect Status (CCS) and Connect Status Change (CSC) in
>> Port status register after an attach.
>> So increasing some timeout when both CCS and CSC bits are not set.
>>
>> Signed-off-by: Amar <amarendra.xt@samsung.com>
>> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
>
> Can we not postpone checking of these CCS and CSC bits for such broken devices?
> This'd at least allow the "good" devices to be detected quickly and while these
> are handled, this would give some time for the "bad" ones to do their job too.
>
> Best regards,
> Marek Vasut
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot



-- 
Thanks & Regards
Vivek

[U-Boot] [PATCH 2/7] usb: hub: Conditionally power on usb's root-hub ports

[Vivek Gautam]

Hi Marek,

Adding CC: Amarendra

On Thu, Mar 28, 2013 at 7:58 PM, Marek Vasut <marex@denx.de> wrote:
> Dear Vivek Gautam,
>
>> Power on root hubs' ports only when they are not yet powered on.
>> Its seen with USB 3.0 ports that they are powered on after
>> a H/W reset, as also reflected in XHCI spec (sec 4.3):
>> "After a Chip Hardware Reset, HCRST, or commanded to the
>> PLS = RxDetect state, all Root Hub ports shall be in Disconnected
>> state, i.e. the port is powered on (PP = 1)"
>>
>> Signed-off-by: Amar <amarendra.xt@samsung.com>
>> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
>
> Just wondering if we shouldn't power-cycle all the ports instead -- aka. turn
> them off and then turn all of them on again.

Hmm, seems like a fine idea.
Will it be fine to power-cycle even EHCI ports too ?
Or we just check if ports are already powered-up (as in XHCI) we
power-cycle them.



-- 
Thanks & Regards
Vivek

[U-Boot] [PATCH 6/7] USB: SS: Add support for Super Speed USB interface

[Vivek Gautam]

Hi Marek,


On Thu, Mar 28, 2013 at 8:05 PM, Marek Vasut <marex@denx.de> wrote:
> Dear Vivek Gautam,
>
>> This adds usb framework support for super-speed usb, which will
>> further facilitate to add stack support for xHCI.
>>
>> Signed-off-by: Vikas C Sajjan <vikas.sajjan@samsung.com>
>> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
>
> [...]
>
>> --- a/include/usb.h
>> +++ b/include/usb.h
>> @@ -67,6 +67,16 @@ struct devrequest {
>>       unsigned short  length;
>>  } __attribute__ ((packed));
>>
>> +struct usb_ep_desc {
>> +     struct usb_endpoint_descriptor          ep_desc;
>> +     /*
>> +      * Super Speed Device will have Super Speed Endpoint
>> +      * Companion Descriptor  (section 9.6.7 of usb 3.0 spec)
>> +      * Revision 1.0 June 6th 2011
>> +      */
>> +     struct usb_ss_ep_comp_descriptor        ss_ep_comp;
>> +};
>> +
>>  /* Interface */
>>  struct usb_interface {
>>       struct usb_interface_descriptor desc;
>> @@ -75,7 +85,7 @@ struct usb_interface {
>>       unsigned char   num_altsetting;
>>       unsigned char   act_altsetting;
>>
>> -     struct usb_endpoint_descriptor ep_desc[USB_MAXENDPOINTS];
>> +     struct usb_ep_desc ep_desc[USB_MAXENDPOINTS];
>
> Do we really need this struct usb_ep_desc? Can we not just store the
> usb_ss_ep_comp_descriptor here as well?

Yea, possibly we can add "usb_ss_ep_comp_descriptor" in
'usb_interface' itself only.
Also quoting USB 3.0 specs:
"SuperSpeed devices shall return Endpoint Companion descriptors
for each of the endpoints in that interface to return additional
information about its endpoint
capabilities. The Endpoint Companion descriptor shall immediately
follow the endpoint descriptor
it is associated with in the configuration information."

Will update this accordingly.

>
>>  } __attribute__ ((packed));
>>
>>  /* Configuration information.. */
>> diff --git a/include/usb_defs.h b/include/usb_defs.h
>> index 0c78d9d..e2aaef3 100644
>> --- a/include/usb_defs.h
>> +++ b/include/usb_defs.h
>> @@ -203,6 +203,8 @@
>>  #define USB_PORT_FEAT_POWER          8
>>  #define USB_PORT_FEAT_LOWSPEED       9
>>  #define USB_PORT_FEAT_HIGHSPEED      10
>> +#define USB_PORT_FEAT_FULLSPEED      11
>> +#define USB_PORT_FEAT_SUPERSPEED     12
>>  #define USB_PORT_FEAT_C_CONNECTION   16
>>  #define USB_PORT_FEAT_C_ENABLE       17
>>  #define USB_PORT_FEAT_C_SUSPEND      18
>> @@ -218,8 +220,20 @@
>>  #define USB_PORT_STAT_POWER         0x0100
>>  #define USB_PORT_STAT_LOW_SPEED     0x0200
>>  #define USB_PORT_STAT_HIGH_SPEED    0x0400   /* support for EHCI */
>> +#define USB_PORT_STAT_FULL_SPEED    0x0800
>> +#define USB_PORT_STAT_SUPER_SPEED   0x1000   /* support for XHCI */
>>  #define USB_PORT_STAT_SPEED  \
>> -     (USB_PORT_STAT_LOW_SPEED | USB_PORT_STAT_HIGH_SPEED)
>> +     (USB_PORT_STAT_LOW_SPEED | USB_PORT_STAT_HIGH_SPEED | \
>> +     USB_PORT_STAT_FULL_SPEED | USB_PORT_STAT_SUPER_SPEED)
>> +
>> +/*
>> + * Additions to wPortStatus bit field from USB 3.0
>> + * See USB 3.0 spec Table 10-10
>> + */
>> +#define USB_PORT_STAT_LINK_STATE     0x01e0
>> +#define USB_SS_PORT_STAT_POWER               0x0200
>> +#define USB_SS_PORT_STAT_SPEED               0x1c00
>> +#define USB_PORT_STAT_SPEED_5GBPS    0x0000
>>
>>  /* wPortChange bits */
>>  #define USB_PORT_STAT_C_CONNECTION  0x0001
>> @@ -228,6 +242,14 @@
>>  #define USB_PORT_STAT_C_OVERCURRENT 0x0008
>>  #define USB_PORT_STAT_C_RESET       0x0010
>>
>> +/*
>> + * Addition to wPortChange bit fields form USB 3.0
>> + * See USB 3.0 spec Table 10-11
>> + */
>> +#define USB_PORT_STAT_C_BH_RESET     0x0020
>> +#define USB_PORT_STAT_C_LINK_STATE   0x0040
>> +#define USB_PORT_STAT_C_CONFIG_ERROR 0x0080
>> +
>>  /* wHubCharacteristics (masks) */
>>  #define HUB_CHAR_LPSM               0x0003
>>  #define HUB_CHAR_COMPOUND           0x0004


-- 
Thanks & Regards
Vivek

[U-Boot] [PATCH 2/7] usb: hub: Conditionally power on usb's root-hub ports

[Marek Vasut]

Dear Vivek Gautam,

> Hi Marek,
> 
> Adding CC: Amarendra
> 
> On Thu, Mar 28, 2013 at 7:58 PM, Marek Vasut <marex@denx.de> wrote:
> > Dear Vivek Gautam,
> > 
> >> Power on root hubs' ports only when they are not yet powered on.
> >> Its seen with USB 3.0 ports that they are powered on after
> >> a H/W reset, as also reflected in XHCI spec (sec 4.3):
> >> "After a Chip Hardware Reset, HCRST, or commanded to the
> >> PLS = RxDetect state, all Root Hub ports shall be in Disconnected
> >> state, i.e. the port is powered on (PP = 1)"
> >> 
> >> Signed-off-by: Amar <amarendra.xt@samsung.com>
> >> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
> > 
> > Just wondering if we shouldn't power-cycle all the ports instead -- aka.
> > turn them off and then turn all of them on again.
> 
> Hmm, seems like a fine idea.
> Will it be fine to power-cycle even EHCI ports too ?
> Or we just check if ports are already powered-up (as in XHCI) we
> power-cycle them.

EHCI as well, all of them without differentiating the type. Why not , do you see 
a problem ?

Best regards,
Marek Vasut

[U-Boot] [PATCH 3/7] usb: Update device class in usb device's descriptor

[Marek Vasut]

Dear Vivek Gautam,

> CC: Amarendra
> 
> On Thu, Mar 28, 2013 at 7:58 PM, Marek Vasut <marex@denx.de> wrote:
> > Dear Vivek Gautam,
> > 
> >> Fetch the device class into usb device's dwcriptors,
> >> so that the host controller's driver can use this info
> >> to differentiate between HUB and DEVICE.
> >> 
> >> Signed-off-by: Amar <amarendra.xt@samsung.com>
> > 
> > Is this a full name? Otherwise this patch is OK.
> > 
> >> ---
> >> 
> >>  common/usb.c |    5 +++++
> >>  1 files changed, 5 insertions(+), 0 deletions(-)
> >> 
> >> diff --git a/common/usb.c b/common/usb.c
> >> index 40c1547..39fcedd 100644
> >> --- a/common/usb.c
> >> +++ b/common/usb.c
> >> @@ -888,6 +888,11 @@ int usb_new_device(struct usb_device *dev)
> >> 
> >>       dev->descriptor.bMaxPacketSize0 = desc->bMaxPacketSize0;
> >> 
> >> +     /* Fetch the device class, driver can use this info
> >> +      * to differentiate between HUB and DEVICE.
> >> +      */
> >> +     dev->descriptor.bDeviceClass = desc->bDeviceClass;
> >> +

btw this really needs fixing, both the comment and the dangling newline.

> >>       /* find the port number we're at */
> >>       if (parent) {
> >>       
> >>               int j;
> > 
> > Best regards,
> > Marek Vasut

Best regards,
Marek Vasut

[U-Boot] [PATCH 2/7] usb: hub: Conditionally power on usb's root-hub ports

[Vivek Gautam]

Hi,


On Tue, Apr 2, 2013 at 3:39 PM, Marek Vasut <marex@denx.de> wrote:
> Dear Vivek Gautam,
>
>> Hi Marek,
>>
>> Adding CC: Amarendra
>>
>> On Thu, Mar 28, 2013 at 7:58 PM, Marek Vasut <marex@denx.de> wrote:
>> > Dear Vivek Gautam,
>> >
>> >> Power on root hubs' ports only when they are not yet powered on.
>> >> Its seen with USB 3.0 ports that they are powered on after
>> >> a H/W reset, as also reflected in XHCI spec (sec 4.3):
>> >> "After a Chip Hardware Reset, HCRST, or commanded to the
>> >> PLS = RxDetect state, all Root Hub ports shall be in Disconnected
>> >> state, i.e. the port is powered on (PP = 1)"
>> >>
>> >> Signed-off-by: Amar <amarendra.xt@samsung.com>
>> >> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
>> >
>> > Just wondering if we shouldn't power-cycle all the ports instead -- aka.
>> > turn them off and then turn all of them on again.
>>
>> Hmm, seems like a fine idea.
>> Will it be fine to power-cycle even EHCI ports too ?
>> Or we just check if ports are already powered-up (as in XHCI) we
>> power-cycle them.
>
> EHCI as well, all of them without differentiating the type. Why not , do you see
> a problem ?

Not of course, will amend this accordingly.


-- 
Thanks & Regards
Vivek

[U-Boot] [PATCH 3/7] usb: Update device class in usb device's descriptor

[Vivek Gautam]

Hi,


On Tue, Apr 2, 2013 at 3:39 PM, Marek Vasut <marex@denx.de> wrote:
> Dear Vivek Gautam,
>
>> CC: Amarendra
>>
>> On Thu, Mar 28, 2013 at 7:58 PM, Marek Vasut <marex@denx.de> wrote:
>> > Dear Vivek Gautam,
>> >
>> >> Fetch the device class into usb device's dwcriptors,
>> >> so that the host controller's driver can use this info
>> >> to differentiate between HUB and DEVICE.
>> >>
>> >> Signed-off-by: Amar <amarendra.xt@samsung.com>
>> >
>> > Is this a full name? Otherwise this patch is OK.
>> >
>> >> ---
>> >>
>> >>  common/usb.c |    5 +++++
>> >>  1 files changed, 5 insertions(+), 0 deletions(-)
>> >>
>> >> diff --git a/common/usb.c b/common/usb.c
>> >> index 40c1547..39fcedd 100644
>> >> --- a/common/usb.c
>> >> +++ b/common/usb.c
>> >> @@ -888,6 +888,11 @@ int usb_new_device(struct usb_device *dev)
>> >>
>> >>       dev->descriptor.bMaxPacketSize0 = desc->bMaxPacketSize0;
>> >>
>> >> +     /* Fetch the device class, driver can use this info
>> >> +      * to differentiate between HUB and DEVICE.
>> >> +      */
>> >> +     dev->descriptor.bDeviceClass = desc->bDeviceClass;
>> >> +
>
> btw this really needs fixing, both the comment and the dangling newline.

Sure, will fix this.

>
>> >>       /* find the port number we're at */
>> >>       if (parent) {
>> >>
>> >>               int j;
>> >



-- 
Thanks & Regards
Vivek

[U-Boot] [PATCH 2/7] usb: hub: Conditionally power on usb's root-hub ports

[Marek Vasut]

Dear Vivek Gautam,

> Hi,
> 
> On Tue, Apr 2, 2013 at 3:39 PM, Marek Vasut <marex@denx.de> wrote:
> > Dear Vivek Gautam,
> > 
> >> Hi Marek,
> >> 
> >> Adding CC: Amarendra
> >> 
> >> On Thu, Mar 28, 2013 at 7:58 PM, Marek Vasut <marex@denx.de> wrote:
> >> > Dear Vivek Gautam,
> >> > 
> >> >> Power on root hubs' ports only when they are not yet powered on.
> >> >> Its seen with USB 3.0 ports that they are powered on after
> >> >> a H/W reset, as also reflected in XHCI spec (sec 4.3):
> >> >> "After a Chip Hardware Reset, HCRST, or commanded to the
> >> >> PLS = RxDetect state, all Root Hub ports shall be in Disconnected
> >> >> state, i.e. the port is powered on (PP = 1)"
> >> >> 
> >> >> Signed-off-by: Amar <amarendra.xt@samsung.com>
> >> >> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
> >> > 
> >> > Just wondering if we shouldn't power-cycle all the ports instead --
> >> > aka. turn them off and then turn all of them on again.
> >> 
> >> Hmm, seems like a fine idea.
> >> Will it be fine to power-cycle even EHCI ports too ?
> >> Or we just check if ports are already powered-up (as in XHCI) we
> >> power-cycle them.
> > 
> > EHCI as well, all of them without differentiating the type. Why not , do
> > you see a problem ?
> 
> Not of course, will amend this accordingly.

Sounds great! Thank you!

Best regards,
Marek Vasut

[U-Boot] [PATCH 5/7] usb: hub: Increase device enumeration timeout for broken drives

[Vivek Gautam]

Hi Marek,


On Thu, Mar 28, 2013 at 8:02 PM, Marek Vasut <marex@denx.de> wrote:
> Dear Vivek Gautam,
>
>> Few broken usb mass storage devices can take some time to set
>> Current Connect Status (CCS) and Connect Status Change (CSC) in
>> Port status register after an attach.
>> So increasing some timeout when both CCS and CSC bits are not set.
>>
>> Signed-off-by: Amar <amarendra.xt@samsung.com>
>> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
>
> Can we not postpone checking of these CCS and CSC bits for such broken devices?
> This'd at least allow the "good" devices to be detected quickly and while these
> are handled, this would give some time for the "bad" ones to do their job too.

We are thinking of one approach here.
Iterating over all the ports once, so that we find 'good' ports and 'bad' ports
By 'bad' port we mean that either the device connected to it is bad or
nothing is connected
to this port.
Once we are done with "usb_hub_port_connect_change()" and other jobs
for 'good' ports,
we iterate once again on bad ports.
Not sure how good this approach :-(.  Any suggestions ?



-- 
Thanks & Regards
Vivek

[U-Boot] [PATCH 5/7] usb: hub: Increase device enumeration timeout for broken drives

[Marek Vasut]

Dear Vivek Gautam,

> Hi Marek,
> 
> On Thu, Mar 28, 2013 at 8:02 PM, Marek Vasut <marex@denx.de> wrote:
> > Dear Vivek Gautam,
> > 
> >> Few broken usb mass storage devices can take some time to set
> >> Current Connect Status (CCS) and Connect Status Change (CSC) in
> >> Port status register after an attach.
> >> So increasing some timeout when both CCS and CSC bits are not set.
> >> 
> >> Signed-off-by: Amar <amarendra.xt@samsung.com>
> >> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
> > 
> > Can we not postpone checking of these CCS and CSC bits for such broken
> > devices? This'd at least allow the "good" devices to be detected quickly
> > and while these are handled, this would give some time for the "bad"
> > ones to do their job too.
> 
> We are thinking of one approach here.
> Iterating over all the ports once, so that we find 'good' ports and 'bad'
> ports By 'bad' port we mean that either the device connected to it is bad
> or nothing is connected
> to this port.
> Once we are done with "usb_hub_port_connect_change()" and other jobs
> for 'good' ports,
> we iterate once again on bad ports.
> Not sure how good this approach :-(.  Any suggestions ?

That might just work, but I wonder if it's worth it. It's a pity we can't make a 
list of those as they're not even probed yet :(

Best regards,
Marek Vasut

[U-Boot] [PATCH 5/7] usb: hub: Increase device enumeration timeout for broken drives

[Vivek Gautam]

Hi Marek,


On Thu, Apr 4, 2013 at 9:28 AM, Marek Vasut <marex@denx.de> wrote:
> Dear Vivek Gautam,
>
>> Hi Marek,
>>
>> On Thu, Mar 28, 2013 at 8:02 PM, Marek Vasut <marex@denx.de> wrote:
>> > Dear Vivek Gautam,
>> >
>> >> Few broken usb mass storage devices can take some time to set
>> >> Current Connect Status (CCS) and Connect Status Change (CSC) in
>> >> Port status register after an attach.
>> >> So increasing some timeout when both CCS and CSC bits are not set.
>> >>
>> >> Signed-off-by: Amar <amarendra.xt@samsung.com>
>> >> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
>> >
>> > Can we not postpone checking of these CCS and CSC bits for such broken
>> > devices? This'd at least allow the "good" devices to be detected quickly
>> > and while these are handled, this would give some time for the "bad"
>> > ones to do their job too.
>>
>> We are thinking of one approach here.
>> Iterating over all the ports once, so that we find 'good' ports and 'bad'
>> ports By 'bad' port we mean that either the device connected to it is bad
>> or nothing is connected
>> to this port.
>> Once we are done with "usb_hub_port_connect_change()" and other jobs
>> for 'good' ports,
>> we iterate once again on bad ports.
>> Not sure how good this approach :-(.  Any suggestions ?
>
> That might just work, but I wonder if it's worth it. It's a pity we can't make a
> list of those as they're not even probed yet :(

True, but at this point of time atleast the CSC and CCS bits tell us
about whether something is connected or not. Right ?
So lets make the ports on which nothing is connected 'bad' and probe
them later so that if something is connected to these,
job can be done.

If you are fine i may drop this patch from this series for now and
come back with a solution.
By the time i can update the patch-series.



-- 
Thanks & Regards
Vivek

[U-Boot] [PATCH 5/7] usb: hub: Increase device enumeration timeout for broken drives

[Marek Vasut]

Dear Vivek Gautam,

> Hi Marek,
> 
> On Thu, Apr 4, 2013 at 9:28 AM, Marek Vasut <marex@denx.de> wrote:
> > Dear Vivek Gautam,
> > 
> >> Hi Marek,
> >> 
> >> On Thu, Mar 28, 2013 at 8:02 PM, Marek Vasut <marex@denx.de> wrote:
> >> > Dear Vivek Gautam,
> >> > 
> >> >> Few broken usb mass storage devices can take some time to set
> >> >> Current Connect Status (CCS) and Connect Status Change (CSC) in
> >> >> Port status register after an attach.
> >> >> So increasing some timeout when both CCS and CSC bits are not set.
> >> >> 
> >> >> Signed-off-by: Amar <amarendra.xt@samsung.com>
> >> >> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
> >> > 
> >> > Can we not postpone checking of these CCS and CSC bits for such broken
> >> > devices? This'd at least allow the "good" devices to be detected
> >> > quickly and while these are handled, this would give some time for
> >> > the "bad" ones to do their job too.
> >> 
> >> We are thinking of one approach here.
> >> Iterating over all the ports once, so that we find 'good' ports and
> >> 'bad' ports By 'bad' port we mean that either the device connected to
> >> it is bad or nothing is connected
> >> to this port.
> >> Once we are done with "usb_hub_port_connect_change()" and other jobs
> >> for 'good' ports,
> >> we iterate once again on bad ports.
> >> Not sure how good this approach :-(.  Any suggestions ?
> > 
> > That might just work, but I wonder if it's worth it. It's a pity we can't
> > make a list of those as they're not even probed yet :(
> 
> True, but at this point of time atleast the CSC and CCS bits tell us
> about whether something is connected or not. Right ?
> So lets make the ports on which nothing is connected 'bad' and probe
> them later so that if something is connected to these,
> job can be done.
> 
> If you are fine i may drop this patch from this series for now and
> come back with a solution.
> By the time i can update the patch-series.

Sorry for the late reply again. I think the best way around here would be to 
make the delay configurable, possibly as an argument to "usb reset" with some 
reasonable default value, what do you think?

Best regards,
Marek Vasut