This patch adds SS support to the dummy hcd module. It may be used to test
SS device when no (SS) HW is available.
USB 3.0 hub includes 2 hubs - HS and SS ones.
This patch adds support for a SS hub in the dummy_hcd module. Thus, when
dummy_hcd enabled it will register 2 root hubs (SS and HS).
A new module parameter was added to simulate a SuperSpeed connection.
When a new device is connected it will enumerate via the HS hub by default.
In order to simulate SuperSpeed connection set the is_super_speed module
parameter to true.
Signed-off-by: Tatyana Brokhman <[email protected]>
---
drivers/usb/gadget/dummy_hcd.c | 540 +++++++++++++++++++++++++++++++++++-----
1 files changed, 483 insertions(+), 57 deletions(-)
diff --git a/drivers/usb/gadget/dummy_hcd.c b/drivers/usb/gadget/dummy_hcd.c
index bf7981d..c2731d3 100644
--- a/drivers/usb/gadget/dummy_hcd.c
+++ b/drivers/usb/gadget/dummy_hcd.c
@@ -70,6 +70,15 @@ MODULE_DESCRIPTION (DRIVER_DESC);
MODULE_AUTHOR ("David Brownell");
MODULE_LICENSE ("GPL");
+struct dummy_hcd_module_parameters {
+ bool is_super_speed;
+};
+
+static struct dummy_hcd_module_parameters mod_data = {
+ .is_super_speed = false
+};
+module_param_named(is_super_speed, mod_data.is_super_speed, bool, S_IRUGO);
+MODULE_PARM_DESC(is_super_speed, "true to simulate SuperSpeed connection");
/*-------------------------------------------------------------------------*/
/* gadget side driver data structres */
@@ -188,6 +197,7 @@ struct dummy {
* MASTER/HOST side support
*/
struct dummy_hcd *hs_hcd;
+ struct dummy_hcd *ss_hcd;
};
static inline struct dummy_hcd *hcd_to_dummy_hcd(struct usb_hcd *hcd)
@@ -218,7 +228,10 @@ static inline struct dummy *ep_to_dummy (struct dummy_ep *ep)
static inline struct dummy_hcd *gadget_to_dummy_hcd(struct usb_gadget *gadget)
{
struct dummy *dum = container_of(gadget, struct dummy, gadget);
- return dum->hs_hcd;
+ if (dum->gadget.speed == USB_SPEED_SUPER)
+ return dum->ss_hcd;
+ else
+ return dum->hs_hcd;
}
static inline struct dummy *gadget_dev_to_dummy (struct device *dev)
@@ -266,10 +279,88 @@ stop_activity (struct dummy *dum)
/* driver now does any non-usb quiescing necessary */
}
+/**
+ * set_ss_link_state() - Sets the current state of the SuperSpeed link
+ * @dum_hcd: pointer to the dummy_hcd structure to update the link
+ * state for
+ *
+ * This function updates the port_status according to the link
+ * state. The old status is saved befor updating.
+ * Note: this function should be called only for SuperSpeed
+ * master and the caller must hold the lock.
+ */
+static void
+set_ss_link_state(struct dummy_hcd *dum_hcd)
+{
+ struct dummy *dum = dum_hcd->dum;
+ if (dum->gadget.speed < USB_SPEED_SUPER)
+ return;
+ dum_hcd->active = 0;
+ if ((dum_hcd->port_status & USB_SS_PORT_STAT_POWER) == 0)
+ dum_hcd->port_status = 0;
+
+ /* UDC suspend must cause a disconnect */
+ else if (!dum->pullup || dum->udc_suspended) {
+ dum_hcd->port_status &= ~(USB_PORT_STAT_CONNECTION |
+ USB_PORT_STAT_ENABLE);
+ if ((dum_hcd->old_status & USB_PORT_STAT_CONNECTION) != 0)
+ dum_hcd->port_status |=
+ (USB_PORT_STAT_C_CONNECTION << 16);
+ } else {
+ /* device is connected and not suspended */
+ dum_hcd->port_status |= (USB_PORT_STAT_CONNECTION |
+ USB_PORT_STAT_SPEED_5GBPS) ;
+ if ((dum_hcd->old_status & USB_PORT_STAT_CONNECTION) == 0)
+ dum_hcd->port_status |=
+ (USB_PORT_STAT_C_CONNECTION << 16);
+ if ((dum_hcd->port_status & USB_PORT_STAT_ENABLE) == 1 &&
+ (dum_hcd->port_status & USB_SS_PORT_LS_U0) == 1 &&
+ dum_hcd->rh_state != DUMMY_RH_SUSPENDED)
+ dum_hcd->active = 1;
+ }
+
+
+ if ((dum_hcd->port_status & USB_PORT_STAT_ENABLE) == 0 ||
+ dum_hcd->active)
+ dum_hcd->resuming = 0;
+
+ /* if !connected or reset */
+ if ((dum_hcd->port_status & USB_PORT_STAT_CONNECTION) == 0 ||
+ (dum_hcd->port_status & USB_PORT_STAT_RESET) != 0) {
+ /*
+ * We're connected and not reseted (reset occured now),
+ * and driver attached - disconnect!
+ */
+ if ((dum_hcd->old_status & USB_PORT_STAT_CONNECTION) != 0 &&
+ (dum_hcd->old_status & USB_PORT_STAT_RESET) == 0 &&
+ dum->driver) {
+ stop_activity(dum);
+ spin_unlock(&dum->lock);
+ dum->driver->disconnect(&dum->gadget);
+ spin_lock(&dum->lock);
+ }
+ } else if (dum_hcd->active != dum_hcd->old_active) {
+ if (dum_hcd->old_active && dum->driver->suspend) {
+ spin_unlock(&dum->lock);
+ dum->driver->suspend(&dum->gadget);
+ spin_lock(&dum->lock);
+ } else if (!dum_hcd->old_active && dum->driver->resume) {
+ spin_unlock(&dum->lock);
+ dum->driver->resume(&dum->gadget);
+ spin_lock(&dum->lock);
+ }
+ }
+
+ dum_hcd->old_status = dum_hcd->port_status;
+ dum_hcd->old_active = dum_hcd->active;
+}
+
/* caller must hold lock */
static void set_link_state(struct dummy_hcd *dum_hcd)
{
struct dummy *dum = dum_hcd->dum;
+ if (dum->gadget.speed == USB_SPEED_SUPER)
+ return;
dum_hcd->active = 0;
if ((dum_hcd->port_status & USB_PORT_STAT_POWER) == 0)
@@ -355,13 +446,17 @@ dummy_enable (struct usb_ep *_ep, const struct usb_endpoint_descriptor *desc)
dum = ep_to_dummy (ep);
if (!dum->driver)
return -ESHUTDOWN;
- dum_hcd = dum->hs_hcd;
+ if (dum->gadget.speed == USB_SPEED_SUPER)
+ dum_hcd = dum->ss_hcd;
+ else
+ dum_hcd = dum->hs_hcd;
if (!is_enabled(dum_hcd))
return -ESHUTDOWN;
/*
* For HS/FS devices only bits 0..10 of the wMaxPacketSize represent the
* maximum packet size.
+ * For SS devices the wMaxPacketSize is limited by 1024.
*/
max = le16_to_cpu(desc->wMaxPacketSize) & 0x7ff;
@@ -381,6 +476,10 @@ dummy_enable (struct usb_ep *_ep, const struct usb_endpoint_descriptor *desc)
goto done;
}
switch (dum->gadget.speed) {
+ case USB_SPEED_SUPER:
+ if (max == 1024)
+ break;
+ goto done;
case USB_SPEED_HIGH:
if (max == 512)
break;
@@ -399,6 +498,7 @@ dummy_enable (struct usb_ep *_ep, const struct usb_endpoint_descriptor *desc)
goto done;
/* real hardware might not handle all packet sizes */
switch (dum->gadget.speed) {
+ case USB_SPEED_SUPER:
case USB_SPEED_HIGH:
if (max <= 1024)
break;
@@ -419,6 +519,7 @@ dummy_enable (struct usb_ep *_ep, const struct usb_endpoint_descriptor *desc)
goto done;
/* real hardware might not handle all packet sizes */
switch (dum->gadget.speed) {
+ case USB_SPEED_SUPER:
case USB_SPEED_HIGH:
if (max <= 1024)
break;
@@ -539,7 +640,10 @@ dummy_queue (struct usb_ep *_ep, struct usb_request *_req,
return -EINVAL;
dum = ep_to_dummy (ep);
- dum_hcd = dum->hs_hcd;
+ if (dum->gadget.speed == USB_SPEED_SUPER)
+ dum_hcd = dum->ss_hcd;
+ else
+ dum_hcd = dum->hs_hcd;
if (!dum->driver || !is_enabled(dum_hcd))
return -ESHUTDOWN;
@@ -725,9 +829,14 @@ static int dummy_pullup (struct usb_gadget *_gadget, int value)
dum = gadget_to_dummy_hcd(_gadget)->dum;
spin_lock_irqsave (&dum->lock, flags);
dum->pullup = (value != 0);
- set_link_state(dum->hs_hcd);
+ if (dum->gadget.speed == USB_SPEED_SUPER)
+ set_ss_link_state(dum->ss_hcd);
+ else
+ set_link_state(dum->hs_hcd);
spin_unlock_irqrestore (&dum->lock, flags);
- usb_hcd_poll_rh_status(dummy_hcd_to_hcd(dum->hs_hcd));
+ usb_hcd_poll_rh_status((dum->gadget.speed == USB_SPEED_SUPER ?
+ dummy_hcd_to_hcd(dum->ss_hcd) :
+ dummy_hcd_to_hcd(dum->hs_hcd)));
return 0;
}
@@ -808,8 +917,21 @@ usb_gadget_probe_driver(struct usb_gadget_driver *driver,
}
dum->gadget.ep0 = &dum->ep [0].ep;
- dum->gadget.speed = min((u8)driver->speed, (u8)USB_SPEED_HIGH) ;
- dum->ep[0].ep.maxpacket = 64;
+ if (mod_data.is_super_speed)
+ dum->gadget.speed = driver->speed;
+ else
+ dum->gadget.speed = min((u8)USB_SPEED_HIGH, (u8)driver->speed);
+ if (dum->gadget.speed < driver->speed)
+ dev_dbg(udc_dev(dum), "This device can perform faster if"
+ " you connect it to a "
+ "SupeSpeed port...\n");
+
+ if (dum->gadget.speed == USB_SPEED_SUPER) {
+ for (i = 0; i < DUMMY_ENDPOINTS; i++)
+ dum->ep[i].ep.max_streams = 0x10;
+ dum->ep[0].ep.maxpacket = 9;
+ } else
+ dum->ep[0].ep.maxpacket = 64;
list_del_init (&dum->ep [0].ep.ep_list);
INIT_LIST_HEAD(&dum->fifo_req.queue);
@@ -828,12 +950,21 @@ usb_gadget_probe_driver(struct usb_gadget_driver *driver,
/* khubd will enumerate this in a while */
spin_lock_irq (&dum->lock);
dum->pullup = 1;
- dum->gadget.is_otg =
- (dummy_hcd_to_hcd(dum->hs_hcd)->self.otg_port != 0);
- set_link_state(dum->hs_hcd);
+ if (dum->gadget.speed == USB_SPEED_SUPER) {
+ dum->gadget.is_otg =
+ (dummy_hcd_to_hcd(dum->ss_hcd)->self.otg_port != 0);
+ set_ss_link_state(dum->ss_hcd);
+ } else {
+ dum->gadget.is_otg =
+ (dummy_hcd_to_hcd(dum->hs_hcd)->self.otg_port != 0);
+ set_link_state(dum->hs_hcd);
+ }
+
spin_unlock_irq (&dum->lock);
- usb_hcd_poll_rh_status(dummy_hcd_to_hcd(dum->hs_hcd));
+ usb_hcd_poll_rh_status((dum->gadget.speed == USB_SPEED_SUPER ?
+ dummy_hcd_to_hcd(dum->ss_hcd) :
+ dummy_hcd_to_hcd(dum->hs_hcd)));
return 0;
}
EXPORT_SYMBOL(usb_gadget_probe_driver);
@@ -854,18 +985,25 @@ usb_gadget_unregister_driver (struct usb_gadget_driver *driver)
spin_lock_irqsave (&dum->lock, flags);
dum->pullup = 0;
- set_link_state(dum->hs_hcd);
+ if (dum->gadget.speed == USB_SPEED_SUPER)
+ set_ss_link_state(dum->ss_hcd);
+ else
+ set_link_state(dum->hs_hcd);
spin_unlock_irqrestore (&dum->lock, flags);
driver->unbind (&dum->gadget);
dum->gadget.dev.driver = NULL;
dum->driver = NULL;
-
spin_lock_irqsave (&dum->lock, flags);
dum->pullup = 0;
- set_link_state(dum->hs_hcd);
+ if (dum->gadget.speed == USB_SPEED_SUPER)
+ set_ss_link_state(dum->ss_hcd);
+ else
+ set_link_state(dum->hs_hcd);
spin_unlock_irqrestore (&dum->lock, flags);
- usb_hcd_poll_rh_status(dummy_hcd_to_hcd(dum->hs_hcd));
+ usb_hcd_poll_rh_status((dum->gadget.speed == USB_SPEED_SUPER ?
+ dummy_hcd_to_hcd(dum->ss_hcd) :
+ dummy_hcd_to_hcd(dum->hs_hcd)));
return 0;
}
EXPORT_SYMBOL (usb_gadget_unregister_driver);
@@ -933,10 +1071,15 @@ static int dummy_udc_suspend (struct platform_device *pdev, pm_message_t state)
dev_dbg (&pdev->dev, "%s\n", __func__);
spin_lock_irq (&dum->lock);
dum->udc_suspended = 1;
- set_link_state(dum->hs_hcd);
+ if (dum->gadget.speed == USB_SPEED_SUPER)
+ set_ss_link_state(dum->ss_hcd);
+ else
+ set_link_state(dum->hs_hcd);
spin_unlock_irq (&dum->lock);
- usb_hcd_poll_rh_status(dummy_hcd_to_hcd(dum->hs_hcd));
+ usb_hcd_poll_rh_status((dum->gadget.speed == USB_SPEED_SUPER ?
+ dummy_hcd_to_hcd(dum->ss_hcd) :
+ dummy_hcd_to_hcd(dum->hs_hcd)));
return 0;
}
@@ -947,10 +1090,15 @@ static int dummy_udc_resume (struct platform_device *pdev)
dev_dbg (&pdev->dev, "%s\n", __func__);
spin_lock_irq (&dum->lock);
dum->udc_suspended = 0;
- set_link_state(dum->hs_hcd);
+ if (dum->gadget.speed == USB_SPEED_SUPER)
+ set_ss_link_state(dum->ss_hcd);
+ else
+ set_link_state(dum->hs_hcd);
spin_unlock_irq (&dum->lock);
- usb_hcd_poll_rh_status(dummy_hcd_to_hcd(dum->hs_hcd));
+ usb_hcd_poll_rh_status((dum->gadget.speed == USB_SPEED_SUPER ?
+ dummy_hcd_to_hcd(dum->ss_hcd) :
+ dummy_hcd_to_hcd(dum->hs_hcd)));
return 0;
}
@@ -1178,6 +1326,21 @@ static int periodic_bytes (struct dummy *dum, struct dummy_ep *ep)
tmp *= 8 /* applies to entire frame */;
limit += limit * tmp;
}
+ if (dum->gadget.speed == USB_SPEED_SUPER) {
+ switch (ep->desc->bmAttributes & 0x03) {
+ case USB_ENDPOINT_XFER_ISOC:
+ /* Sec. 4.4.8.2 USB3.0 Spec */
+ limit = 3 * 16 * 1024 * 8;
+ break;
+ case USB_ENDPOINT_XFER_INT:
+ /* Sec. 4.4.7.2 USB3.0 Spec */
+ limit = 3 * 1024 * 8;
+ break;
+ case USB_ENDPOINT_XFER_BULK:
+ default:
+ break;
+ }
+ }
return limit;
}
@@ -1190,7 +1353,8 @@ static struct dummy_ep *find_endpoint (struct dummy *dum, u8 address)
{
int i;
- if (!is_active(dum->hs_hcd))
+ if (!is_active((dum->gadget.speed == USB_SPEED_SUPER ?
+ dum->ss_hcd : dum->hs_hcd)))
return NULL;
if ((address & ~USB_DIR_IN) == 0)
return &dum->ep [0];
@@ -1264,6 +1428,27 @@ static int handle_control_request(struct dummy_hcd *dum_hcd, struct urb *urb,
case USB_DEVICE_A_ALT_HNP_SUPPORT:
dum->gadget.a_alt_hnp_support = 1;
break;
+ case USB_DEVICE_U1_ENABLE:
+ if (dummy_hcd_to_hcd(dum_hcd)->speed ==
+ HCD_USB3)
+ w_value = USB_DEV_STAT_U1_ENABLED;
+ else
+ ret_val = -EOPNOTSUPP;
+ break;
+ case USB_DEVICE_U2_ENABLE:
+ if (dummy_hcd_to_hcd(dum_hcd)->speed ==
+ HCD_USB3)
+ w_value = USB_DEV_STAT_U2_ENABLED;
+ else
+ ret_val = -EOPNOTSUPP;
+ break;
+ case USB_DEVICE_LTM_ENABLE:
+ if (dummy_hcd_to_hcd(dum_hcd)->speed ==
+ HCD_USB3)
+ w_value = USB_DEV_STAT_LTM_ENABLED;
+ else
+ ret_val = -EOPNOTSUPP;
+ break;
default:
ret_val = -EOPNOTSUPP;
}
@@ -1290,6 +1475,27 @@ static int handle_control_request(struct dummy_hcd *dum_hcd, struct urb *urb,
case USB_DEVICE_REMOTE_WAKEUP:
w_value = USB_DEVICE_REMOTE_WAKEUP;
break;
+ case USB_DEVICE_U1_ENABLE:
+ if (dummy_hcd_to_hcd(dum_hcd)->speed ==
+ HCD_USB3)
+ w_value = USB_DEV_STAT_U1_ENABLED;
+ else
+ ret_val = -EOPNOTSUPP;
+ break;
+ case USB_DEVICE_U2_ENABLE:
+ if (dummy_hcd_to_hcd(dum_hcd)->speed ==
+ HCD_USB3)
+ w_value = USB_DEV_STAT_U2_ENABLED;
+ else
+ ret_val = -EOPNOTSUPP;
+ break;
+ case USB_DEVICE_LTM_ENABLE:
+ if (dummy_hcd_to_hcd(dum_hcd)->speed ==
+ HCD_USB3)
+ w_value = USB_DEV_STAT_LTM_ENABLED;
+ else
+ ret_val = -EOPNOTSUPP;
+ break;
default:
ret_val = -EOPNOTSUPP;
break;
@@ -1371,6 +1577,10 @@ static void dummy_timer(unsigned long _dum_hcd)
case USB_SPEED_HIGH:
total = 512/*bytes*/ * 13/*packets*/ * 8/*uframes*/;
break;
+ case USB_SPEED_SUPER:
+ /* Bus speed is 500000 bytes/ms, so use a little less */
+ total = 490000;
+ break;
default:
dev_err(dummy_dev(dum_hcd), "bogus device speed\n");
return;
@@ -1588,7 +1798,10 @@ static int dummy_hub_status (struct usb_hcd *hcd, char *buf)
if (dum_hcd->resuming && time_after_eq(jiffies, dum_hcd->re_timeout)) {
dum_hcd->port_status |= (USB_PORT_STAT_C_SUSPEND << 16);
dum_hcd->port_status &= ~USB_PORT_STAT_SUSPEND;
- set_link_state(dum_hcd);
+ if (hcd->speed == HCD_USB3)
+ set_ss_link_state(dum_hcd);
+ else
+ set_link_state(dum_hcd);
}
if ((dum_hcd->port_status & PORT_C_MASK) != 0) {
@@ -1605,6 +1818,18 @@ done:
}
static inline void
+ss_hub_descriptor(struct usb_hub_descriptor *desc)
+{
+ memset(desc, 0, sizeof *desc);
+ desc->bDescriptorType = 0x2a;
+ desc->bDescLength = 12;
+ desc->wHubCharacteristics = cpu_to_le16(0x0001);
+ desc->bNbrPorts = 1;
+ desc->u.ss.bHubHdrDecLat = 0x04; /* Worst case: 0.4 micro sec*/
+ desc->u.ss.DeviceRemovable = 0xffff;
+}
+
+static inline void
hub_descriptor (struct usb_hub_descriptor *desc)
{
memset (desc, 0, sizeof *desc);
@@ -1640,6 +1865,12 @@ static int dummy_hub_control (
case ClearPortFeature:
switch (wValue) {
case USB_PORT_FEAT_SUSPEND:
+ if (hcd->speed == HCD_USB3) {
+ dev_dbg(dummy_dev(dum_hcd),
+ "USB_PORT_FEAT_SUSPEND req not "
+ "supported for USB 3.0 roothub\n");
+ goto error;
+ }
if (dum_hcd->port_status & USB_PORT_STAT_SUSPEND) {
/* 20msec resume signaling */
dum_hcd->resuming = 1;
@@ -1648,16 +1879,37 @@ static int dummy_hub_control (
}
break;
case USB_PORT_FEAT_POWER:
- if (dum_hcd->port_status & USB_PORT_STAT_POWER)
- dev_dbg(dummy_dev(dum_hcd), "power-off\n");
+ if (hcd->speed == HCD_USB3) {
+ if (dum_hcd->port_status & USB_PORT_STAT_POWER)
+ dev_dbg(dummy_dev(dum_hcd),
+ "power-off\n");
+ } else
+ if (dum_hcd->port_status &
+ USB_SS_PORT_STAT_POWER)
+ dev_dbg(dummy_dev(dum_hcd),
+ "power-off\n");
/* FALLS THROUGH */
default:
dum_hcd->port_status &= ~(1 << wValue);
- set_link_state(dum_hcd);
+ if (hcd->speed == HCD_USB3)
+ set_ss_link_state(dum_hcd);
+ else
+ set_link_state(dum_hcd);
}
break;
case GetHubDescriptor:
- hub_descriptor((struct usb_hub_descriptor *) buf);
+ if (hcd->speed == HCD_USB3 &&
+ (wLength < USB_DT_SS_HUB_SIZE ||
+ wValue != (USB_DT_SS_HUB << 8))) {
+ dev_dbg(dummy_dev(dum_hcd),
+ "Wrong hub descriptor type for "
+ "USB 3.0 roothub.\n");
+ goto error;
+ }
+ if (hcd->speed == HCD_USB3)
+ ss_hub_descriptor((struct usb_hub_descriptor *) buf);
+ else
+ hub_descriptor((struct usb_hub_descriptor *) buf);
break;
case GetHubStatus:
*(__le32 *) buf = cpu_to_le32 (0);
@@ -1680,25 +1932,31 @@ static int dummy_hub_control (
dum_hcd->port_status &= ~USB_PORT_STAT_RESET;
if (dum_hcd->dum->pullup) {
dum_hcd->port_status |= USB_PORT_STAT_ENABLE;
- switch (dum_hcd->dum->gadget.speed) {
- case USB_SPEED_HIGH:
- dum_hcd->port_status |=
- USB_PORT_STAT_HIGH_SPEED;
- break;
- case USB_SPEED_LOW:
- dum_hcd->dum->gadget.ep0->
- maxpacket = 8;
- dum_hcd->port_status |=
- USB_PORT_STAT_LOW_SPEED;
- break;
- default:
- dum_hcd->dum->gadget.speed =
- USB_SPEED_FULL;
- break;
+
+ if (hcd->speed < HCD_USB3) {
+ switch (dum_hcd->dum->gadget.speed) {
+ case USB_SPEED_HIGH:
+ dum_hcd->port_status |=
+ USB_PORT_STAT_HIGH_SPEED;
+ break;
+ case USB_SPEED_LOW:
+ dum_hcd->dum->gadget.ep0->
+ maxpacket = 8;
+ dum_hcd->port_status |=
+ USB_PORT_STAT_LOW_SPEED;
+ break;
+ default:
+ dum_hcd->dum->gadget.speed =
+ USB_SPEED_FULL;
+ break;
+ }
}
}
}
- set_link_state(dum_hcd);
+ if (hcd->speed == HCD_USB3)
+ set_ss_link_state(dum_hcd);
+ else
+ set_link_state(dum_hcd);
((__le16 *) buf)[0] = cpu_to_le16 (dum_hcd->port_status);
((__le16 *) buf)[1] = cpu_to_le16 (dum_hcd->port_status >> 16);
break;
@@ -1707,7 +1965,36 @@ static int dummy_hub_control (
break;
case SetPortFeature:
switch (wValue) {
+ case USB_PORT_FEAT_LINK_STATE:
+ if (hcd->speed != HCD_USB3) {
+ dev_dbg(dummy_dev(dum_hcd),
+ "USB_PORT_FEAT_LINK_STATE req not "
+ "supported for USB 2.0 roothub\n");
+ goto error;
+ }
+ /*
+ * Since this is dummy we don't have an actual link so
+ * there is nothing to do for the SET_LINK_STATE cmd
+ */
+ break;
+ case USB_PORT_FEAT_U1_TIMEOUT:
+ case USB_PORT_FEAT_U2_TIMEOUT:
+ /* TODO: add suspend/resume support! */
+ if (hcd->speed != HCD_USB3) {
+ dev_dbg(dummy_dev(dum_hcd),
+ "USB_PORT_FEAT_U1/2_TIMEOUT req not "
+ "supported for USB 2.0 roothub\n");
+ goto error;
+ }
+ break;
case USB_PORT_FEAT_SUSPEND:
+ /* Applicable only for USB2.0 hub */
+ if (hcd->speed == HCD_USB3) {
+ dev_dbg(dummy_dev(dum_hcd),
+ "USB_PORT_FEAT_SUSPEND req not "
+ "supported for USB 3.0 roothub\n");
+ goto error;
+ }
if (dum_hcd->active) {
dum_hcd->port_status |= USB_PORT_STAT_SUSPEND;
@@ -1722,12 +2009,33 @@ static int dummy_hub_control (
}
break;
case USB_PORT_FEAT_POWER:
- dum_hcd->port_status |= USB_PORT_STAT_POWER;
- set_link_state(dum_hcd);
+ if (hcd->speed == HCD_USB3) {
+ dum_hcd->port_status |= USB_SS_PORT_STAT_POWER;
+ set_ss_link_state(dum_hcd);
+ } else {
+ dum_hcd->port_status |= USB_PORT_STAT_POWER;
+ set_link_state(dum_hcd);
+ }
break;
+ case USB_PORT_FEAT_BH_PORT_RESET:
+ /* Applicable only for USB3.0 hub */
+ if (hcd->speed != HCD_USB3) {
+ dev_dbg(dummy_dev(dum_hcd),
+ "USB_PORT_FEAT_BH_PORT_RESET req not "
+ "supported for USB 2.0 roothub\n");
+ goto error;
+ }
+ /* FALLS THROUGH */
case USB_PORT_FEAT_RESET:
/* if it's already enabled, disable */
- dum_hcd->port_status &= ~(USB_PORT_STAT_ENABLE
+ if (hcd->speed == HCD_USB3) {
+ dum_hcd->port_status = 0;
+ dum_hcd->port_status =
+ (USB_SS_PORT_STAT_POWER |
+ USB_PORT_STAT_CONNECTION |
+ USB_PORT_STAT_RESET);
+ } else
+ dum_hcd->port_status &= ~(USB_PORT_STAT_ENABLE
| USB_PORT_STAT_LOW_SPEED
| USB_PORT_STAT_HIGH_SPEED);
/*
@@ -1736,21 +2044,50 @@ static int dummy_hub_control (
*/
dum_hcd->dum->devstatus &=
(1 << USB_DEVICE_SELF_POWERED);
+ /*
+ * FIXME USB3.0: what is the correct reset signaling
+ * interval? Is it still 50msec as for HS?
+ */
dum_hcd->re_timeout = jiffies + msecs_to_jiffies(50);
/* FALLS THROUGH */
default:
- if ((dum_hcd->port_status &
- USB_PORT_STAT_POWER) != 0) {
- dum_hcd->port_status |= (1 << wValue);
- set_link_state(dum_hcd);
- }
+ if (hcd->speed == HCD_USB3) {
+ if ((dum_hcd->port_status &
+ USB_SS_PORT_STAT_POWER) != 0) {
+ dum_hcd->port_status |= (1 << wValue);
+ set_ss_link_state(dum_hcd);
+ }
+ } else
+ if ((dum_hcd->port_status &
+ USB_PORT_STAT_POWER) != 0) {
+ dum_hcd->port_status |= (1 << wValue);
+ set_link_state(dum_hcd);
+ }
+ }
+ break;
+ case GetPortErrorCount:
+ if (hcd->speed != HCD_USB3) {
+ dev_dbg(dummy_dev(dum_hcd),
+ "GetPortErrorCount req not "
+ "supported for USB 2.0 roothub\n");
+ goto error;
+ }
+ /* We'll always return 0 since this is a dummy hub */
+ *(__le32 *) buf = cpu_to_le32(0);
+ break;
+ case SetHubDepth:
+ if (hcd->speed != HCD_USB3) {
+ dev_dbg(dummy_dev(dum_hcd),
+ "SetHubDepth req not supported for "
+ "USB 2.0 roothub\n");
+ goto error;
}
break;
-
default:
dev_dbg(dummy_dev(dum_hcd),
"hub control req%04x v%04x i%04x l%d\n",
typeReq, wValue, wIndex, wLength);
+error:
/* "protocol stall" on error */
retval = -EPIPE;
}
@@ -1769,7 +2106,10 @@ static int dummy_bus_suspend (struct usb_hcd *hcd)
spin_lock_irq(&dum_hcd->dum->lock);
dum_hcd->rh_state = DUMMY_RH_SUSPENDED;
- set_link_state(dum_hcd);
+ if (hcd->speed == HCD_USB3)
+ set_ss_link_state(dum_hcd);
+ else
+ set_link_state(dum_hcd);
hcd->state = HC_STATE_SUSPENDED;
spin_unlock_irq(&dum_hcd->dum->lock);
return 0;
@@ -1787,7 +2127,10 @@ static int dummy_bus_resume (struct usb_hcd *hcd)
rc = -ESHUTDOWN;
} else {
dum_hcd->rh_state = DUMMY_RH_RUNNING;
- set_link_state(dum_hcd);
+ if (hcd->speed == HCD_USB3)
+ set_ss_link_state(dum_hcd);
+ else
+ set_link_state(dum_hcd);
if (!list_empty(&dum_hcd->urbp_list))
mod_timer(&dum_hcd->timer, jiffies);
hcd->state = HC_STATE_RUNNING;
@@ -1811,6 +2154,7 @@ show_urb (char *buf, size_t size, struct urb *urb)
case USB_SPEED_LOW: s = "ls"; break;
case USB_SPEED_FULL: s = "fs"; break;
case USB_SPEED_HIGH: s = "hs"; break;
+ case USB_SPEED_SUPER: s = "ss"; break;
default: s = "?"; break;
}; s; }),
ep, ep ? (usb_pipein (urb->pipe) ? "in" : "out") : "",
@@ -1847,6 +2191,25 @@ show_urbs (struct device *dev, struct device_attribute *attr, char *buf)
}
static DEVICE_ATTR (urbs, S_IRUGO, show_urbs, NULL);
+static int dummy_start_ss(struct dummy_hcd *dum_hcd)
+{
+ init_timer(&dum_hcd->timer);
+ dum_hcd->timer.function = dummy_timer;
+ dum_hcd->timer.data = (unsigned long)dum_hcd;
+ dum_hcd->rh_state = DUMMY_RH_RUNNING;
+ INIT_LIST_HEAD(&dum_hcd->urbp_list);
+ dummy_hcd_to_hcd(dum_hcd)->power_budget = POWER_BUDGET;
+ dummy_hcd_to_hcd(dum_hcd)->state = HC_STATE_RUNNING;
+ dummy_hcd_to_hcd(dum_hcd)->uses_new_polling = 1;
+#ifdef CONFIG_USB_OTG
+ dummy_hcd_to_hcd(dum_hcd)->self.otg_port = 1;
+#endif
+ return 0;
+
+ /* FIXME 'urbs' should be a per-device thing, maybe in usbcore */
+ return device_create_file(dummy_dev(dum_hcd), &dev_attr_urbs);
+}
+
static int dummy_start(struct usb_hcd *hcd)
{
struct dummy_hcd *dum_hcd = hcd_to_dummy_hcd(hcd);
@@ -1856,6 +2219,9 @@ static int dummy_start(struct usb_hcd *hcd)
* talk to one device (the slave side). Also appears in sysfs,
* just like more familiar pci-based HCDs.
*/
+ if (!usb_hcd_is_primary_hcd(hcd))
+ return dummy_start_ss(dum_hcd);
+
spin_lock_init(&dum_hcd->dum->lock);
init_timer(&dum_hcd->timer);
dum_hcd->timer.function = dummy_timer;
@@ -1898,19 +2264,52 @@ static int dummy_setup(struct usb_hcd *hcd)
if (usb_hcd_is_primary_hcd(hcd)) {
the_controller.hs_hcd = hcd_to_dummy_hcd(hcd);
the_controller.hs_hcd->dum = &the_controller;
- /* Mark the first roothub as being USB 2.0. */
+ /*
+ * Mark the first roothub as being USB 2.0.
+ * The USB 3.0 roothub will be registered later by
+ * dummy_hcd_probe()
+ */
hcd->speed = HCD_USB2;
hcd->self.root_hub->speed = USB_SPEED_HIGH;
+ } else {
+ the_controller.ss_hcd = hcd_to_dummy_hcd(hcd);
+ the_controller.ss_hcd->dum = &the_controller;
+ hcd->speed = HCD_USB3;
+ hcd->self.root_hub->speed = USB_SPEED_SUPER;
}
return 0;
}
+/* Change a group of bulk endpoints to support multiple stream IDs */
+int dummy_alloc_streams(struct usb_hcd *hcd, struct usb_device *udev,
+ struct usb_host_endpoint **eps, unsigned int num_eps,
+ unsigned int num_streams, gfp_t mem_flags)
+{
+ if (hcd->speed != HCD_USB3)
+ dev_dbg(dummy_dev(hcd_to_dummy_hcd(hcd)),
+ "%s() - ERROR! Not supported for USB2.0 roothub\n",
+ __func__);
+ return 0;
+}
+
+/* Reverts a group of bulk endpoints back to not using stream IDs. */
+int dummy_free_streams(struct usb_hcd *hcd, struct usb_device *udev,
+ struct usb_host_endpoint **eps, unsigned int num_eps,
+ gfp_t mem_flags)
+{
+ if (hcd->speed != HCD_USB3)
+ dev_dbg(dummy_dev(hcd_to_dummy_hcd(hcd)),
+ "%s() - ERROR! Not supported for USB2.0 roothub\n",
+ __func__);
+ return 0;
+}
+
static const struct hc_driver dummy_hcd = {
.description = (char *) driver_name,
.product_desc = "Dummy host controller",
.hcd_priv_size = sizeof(struct dummy_hcd),
- .flags = HCD_USB2,
+ .flags = HCD_USB3 | HCD_SHARED,
.reset = dummy_setup,
.start = dummy_start,
@@ -1925,11 +2324,15 @@ static const struct hc_driver dummy_hcd = {
.hub_control = dummy_hub_control,
.bus_suspend = dummy_bus_suspend,
.bus_resume = dummy_bus_resume,
+
+ .alloc_streams = dummy_alloc_streams,
+ .free_streams = dummy_free_streams,
};
static int dummy_hcd_probe(struct platform_device *pdev)
{
struct usb_hcd *hs_hcd;
+ struct usb_hcd *ss_hcd;
int retval;
dev_info(&pdev->dev, "%s, driver " DRIVER_VERSION "\n", driver_desc);
@@ -1941,8 +2344,27 @@ static int dummy_hcd_probe(struct platform_device *pdev)
retval = usb_add_hcd(hs_hcd, 0, 0);
if (retval != 0) {
usb_put_hcd(hs_hcd);
- the_controller.hs_hcd = NULL;
+ return retval;
+ }
+
+ ss_hcd = usb_create_shared_hcd(&dummy_hcd, &pdev->dev,
+ dev_name(&pdev->dev), hs_hcd);
+ if (!ss_hcd) {
+ retval = -ENOMEM;
+ goto dealloc_usb2_hcd;
}
+
+ retval = usb_add_hcd(ss_hcd, 0, 0);
+ if (retval)
+ goto put_usb3_hcd;
+
+ return 0;
+
+put_usb3_hcd:
+ usb_put_hcd(ss_hcd);
+dealloc_usb2_hcd:
+ usb_put_hcd(hs_hcd);
+ the_controller.hs_hcd = the_controller.ss_hcd = NULL;
return retval;
}
@@ -1951,9 +2373,13 @@ static int dummy_hcd_remove (struct platform_device *pdev)
struct dummy *dum;
dum = (hcd_to_dummy_hcd(platform_get_drvdata(pdev)))->dum;
+ if (dum->ss_hcd) {
+ usb_remove_hcd(dummy_hcd_to_hcd(dum->ss_hcd));
+ usb_put_hcd(dummy_hcd_to_hcd(dum->ss_hcd));
+ }
usb_remove_hcd(dummy_hcd_to_hcd(dum->hs_hcd));
usb_put_hcd(dummy_hcd_to_hcd(dum->hs_hcd));
- the_controller.hs_hcd = NULL;
+ the_controller.ss_hcd = the_controller.hs_hcd = NULL;
return 0;
}
@@ -2027,7 +2453,7 @@ static int __init init (void)
retval = platform_device_add(the_hcd_pdev);
if (retval < 0)
goto err_add_hcd;
- if (!the_controller.hs_hcd) {
+ if (!the_controller.hs_hcd || !the_controller.ss_hcd) {
/*
* The hcd was added successfully but its probe function failed
* for some reason.
--
1.7.3.3
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Hi,
On Mon, May 23, 2011 at 09:41:16AM +0300, Tatyana Brokhman wrote:
> This patch adds SS support to the dummy hcd module. It may be used to test
> SS device when no (SS) HW is available.
> USB 3.0 hub includes 2 hubs - HS and SS ones.
> This patch adds support for a SS hub in the dummy_hcd module. Thus, when
> dummy_hcd enabled it will register 2 root hubs (SS and HS).
> A new module parameter was added to simulate a SuperSpeed connection.
> When a new device is connected it will enumerate via the HS hub by default.
> In order to simulate SuperSpeed connection set the is_super_speed module
> parameter to true.
>
> Signed-off-by: Tatyana Brokhman <[email protected]>
>
> ---
> drivers/usb/gadget/dummy_hcd.c | 540 +++++++++++++++++++++++++++++++++++-----
> 1 files changed, 483 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/usb/gadget/dummy_hcd.c b/drivers/usb/gadget/dummy_hcd.c
> index bf7981d..c2731d3 100644
> --- a/drivers/usb/gadget/dummy_hcd.c
> +++ b/drivers/usb/gadget/dummy_hcd.c
> @@ -70,6 +70,15 @@ MODULE_DESCRIPTION (DRIVER_DESC);
> MODULE_AUTHOR ("David Brownell");
> MODULE_LICENSE ("GPL");
>
> +struct dummy_hcd_module_parameters {
> + bool is_super_speed;
> +};
> +
> +static struct dummy_hcd_module_parameters mod_data = {
> + .is_super_speed = false
> +};
> +module_param_named(is_super_speed, mod_data.is_super_speed, bool, S_IRUGO);
> +MODULE_PARM_DESC(is_super_speed, "true to simulate SuperSpeed connection");
you shouldn't need this. You should always enable SuperSpeed for this
driver.
> /*-------------------------------------------------------------------------*/
>
> /* gadget side driver data structres */
> @@ -188,6 +197,7 @@ struct dummy {
> * MASTER/HOST side support
> */
> struct dummy_hcd *hs_hcd;
> + struct dummy_hcd *ss_hcd;
> };
>
> static inline struct dummy_hcd *hcd_to_dummy_hcd(struct usb_hcd *hcd)
> @@ -218,7 +228,10 @@ static inline struct dummy *ep_to_dummy (struct dummy_ep *ep)
> static inline struct dummy_hcd *gadget_to_dummy_hcd(struct usb_gadget *gadget)
> {
> struct dummy *dum = container_of(gadget, struct dummy, gadget);
> - return dum->hs_hcd;
> + if (dum->gadget.speed == USB_SPEED_SUPER)
> + return dum->ss_hcd;
> + else
> + return dum->hs_hcd;
> }
>
> static inline struct dummy *gadget_dev_to_dummy (struct device *dev)
> @@ -266,10 +279,88 @@ stop_activity (struct dummy *dum)
> /* driver now does any non-usb quiescing necessary */
> }
>
> +/**
> + * set_ss_link_state() - Sets the current state of the SuperSpeed link
> + * @dum_hcd: pointer to the dummy_hcd structure to update the link
> + * state for
> + *
> + * This function updates the port_status according to the link
> + * state. The old status is saved befor updating.
> + * Note: this function should be called only for SuperSpeed
> + * master and the caller must hold the lock.
> + */
> +static void
> +set_ss_link_state(struct dummy_hcd *dum_hcd)
a significant part of this function is similar to the USB2.0 version
set_link_state(), so it's better to phase out the similar parts to an
internal function and use that on both set_*_link_state(). Add something
like:
__set_link_state_by_speed(struct dummy_hcd *dum_hcd, enum
usb_device_speed speed);
then do all the similar parts there copying for speed, then just call
that function on both set_ss_link_state() and set_link_state().
> +{
> + struct dummy *dum = dum_hcd->dum;
add a blank line here
> + if (dum->gadget.speed < USB_SPEED_SUPER)
this assumes USB_SPEED_SUPER has an assigned value bigger than the
others which might not be true if someone changes the order of the
enumeration. So it's better to use:
if (dum->gadget.speed != USB_SPEED_SUPER)
> + return;
> + dum_hcd->active = 0;
> + if ((dum_hcd->port_status & USB_SS_PORT_STAT_POWER) == 0)
> + dum_hcd->port_status = 0;
if one branch has {} all of them should have :-) You can fix it while
re-factoring this code.
> + /* UDC suspend must cause a disconnect */
> + else if (!dum->pullup || dum->udc_suspended) {
> + dum_hcd->port_status &= ~(USB_PORT_STAT_CONNECTION |
> + USB_PORT_STAT_ENABLE);
> + if ((dum_hcd->old_status & USB_PORT_STAT_CONNECTION) != 0)
> + dum_hcd->port_status |=
> + (USB_PORT_STAT_C_CONNECTION << 16);
> + } else {
> + /* device is connected and not suspended */
> + dum_hcd->port_status |= (USB_PORT_STAT_CONNECTION |
> + USB_PORT_STAT_SPEED_5GBPS) ;
> + if ((dum_hcd->old_status & USB_PORT_STAT_CONNECTION) == 0)
> + dum_hcd->port_status |=
> + (USB_PORT_STAT_C_CONNECTION << 16);
> + if ((dum_hcd->port_status & USB_PORT_STAT_ENABLE) == 1 &&
> + (dum_hcd->port_status & USB_SS_PORT_LS_U0) == 1 &&
> + dum_hcd->rh_state != DUMMY_RH_SUSPENDED)
> + dum_hcd->active = 1;
> + }
> +
> +
only one blank line :-)
> + if ((dum_hcd->port_status & USB_PORT_STAT_ENABLE) == 0 ||
> + dum_hcd->active)
> + dum_hcd->resuming = 0;
> +
> + /* if !connected or reset */
> + if ((dum_hcd->port_status & USB_PORT_STAT_CONNECTION) == 0 ||
> + (dum_hcd->port_status & USB_PORT_STAT_RESET) != 0) {
> + /*
> + * We're connected and not reseted (reset occured now),
'reseted' isn't proper spelling :-)
> + * and driver attached - disconnect!
> + */
> + if ((dum_hcd->old_status & USB_PORT_STAT_CONNECTION) != 0 &&
> + (dum_hcd->old_status & USB_PORT_STAT_RESET) == 0 &&
> + dum->driver) {
> + stop_activity(dum);
> + spin_unlock(&dum->lock);
> + dum->driver->disconnect(&dum->gadget);
> + spin_lock(&dum->lock);
> + }
> + } else if (dum_hcd->active != dum_hcd->old_active) {
> + if (dum_hcd->old_active && dum->driver->suspend) {
> + spin_unlock(&dum->lock);
> + dum->driver->suspend(&dum->gadget);
> + spin_lock(&dum->lock);
> + } else if (!dum_hcd->old_active && dum->driver->resume) {
> + spin_unlock(&dum->lock);
> + dum->driver->resume(&dum->gadget);
> + spin_lock(&dum->lock);
> + }
> + }
> +
> + dum_hcd->old_status = dum_hcd->port_status;
> + dum_hcd->old_active = dum_hcd->active;
> +}
> +
> /* caller must hold lock */
> static void set_link_state(struct dummy_hcd *dum_hcd)
> {
> struct dummy *dum = dum_hcd->dum;
add a blank line here.
> @@ -1371,6 +1577,10 @@ static void dummy_timer(unsigned long _dum_hcd)
> case USB_SPEED_HIGH:
> total = 512/*bytes*/ * 13/*packets*/ * 8/*uframes*/;
> break;
> + case USB_SPEED_SUPER:
> + /* Bus speed is 500000 bytes/ms, so use a little less */
isn't it 500000 bits/ms ?
--
balbi
Hi
> >
> > diff --git a/drivers/usb/gadget/dummy_hcd.c
> > b/drivers/usb/gadget/dummy_hcd.c index bf7981d..c2731d3 100644
> > --- a/drivers/usb/gadget/dummy_hcd.c
> > +++ b/drivers/usb/gadget/dummy_hcd.c
> > @@ -70,6 +70,15 @@ MODULE_DESCRIPTION (DRIVER_DESC); MODULE_AUTHOR
> > ("David Brownell"); MODULE_LICENSE ("GPL");
> >
> > +struct dummy_hcd_module_parameters {
> > + bool is_super_speed;
> > +};
> > +
> > +static struct dummy_hcd_module_parameters mod_data = {
> > + .is_super_speed = false
> > +};
> > +module_param_named(is_super_speed, mod_data.is_super_speed, bool,
> > +S_IRUGO); MODULE_PARM_DESC(is_super_speed, "true to simulate
> > +SuperSpeed connection");
>
> you shouldn't need this. You should always enable SuperSpeed for this
> driver.
You mean I don't need the module parameter? IMO it's the best way to enable
HS connection. If driver->speed=USB_SPEED_SUPER than dummy_hcd will try to
enumerate the device on the SS root hub and if the gadget didn't provide SS
descriptors - it will fail. Just as it happened before. Finding out from
dummy_hcd that the enumeration failed is very complicated (if even possible)
and I'm not sure that is the right thing to do... If you connect a real
device over SS port to xHCI and the device doesn't provide SS descriptors -
the enumeration fails and it's ok. But if you connect the same device to a
HS port - it should work properly. This is what I tried to simulate with
this parameter.
Regarding the default value set to false, this of course can be changed. I
just thought that at the moment most of the existing gadget drivers don't
have SS descriptors so their default operation mode is HS.
Best regards,
Tanya Brokhman
Consultant for Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
Hi,
On Mon, May 23, 2011 at 10:20:45AM +0300, Tanya Brokhman wrote:
> > > diff --git a/drivers/usb/gadget/dummy_hcd.c
> > > b/drivers/usb/gadget/dummy_hcd.c index bf7981d..c2731d3 100644
> > > --- a/drivers/usb/gadget/dummy_hcd.c
> > > +++ b/drivers/usb/gadget/dummy_hcd.c
> > > @@ -70,6 +70,15 @@ MODULE_DESCRIPTION (DRIVER_DESC); MODULE_AUTHOR
> > > ("David Brownell"); MODULE_LICENSE ("GPL");
> > >
> > > +struct dummy_hcd_module_parameters {
> > > + bool is_super_speed;
> > > +};
> > > +
> > > +static struct dummy_hcd_module_parameters mod_data = {
> > > + .is_super_speed = false
> > > +};
> > > +module_param_named(is_super_speed, mod_data.is_super_speed, bool,
> > > +S_IRUGO); MODULE_PARM_DESC(is_super_speed, "true to simulate
> > > +SuperSpeed connection");
> >
> > you shouldn't need this. You should always enable SuperSpeed for this
> > driver.
>
> You mean I don't need the module parameter? IMO it's the best way to enable
> HS connection. If driver->speed=USB_SPEED_SUPER than dummy_hcd will try to
> enumerate the device on the SS root hub and if the gadget didn't provide SS
> descriptors - it will fail. Just as it happened before. Finding out from
then it should hand the device over to the hs_hcd ;-) Meaning it would
disconnect the device, switch to hs_hcd and reconnect :-)
> dummy_hcd that the enumeration failed is very complicated (if even possible)
> and I'm not sure that is the right thing to do... If you connect a real
> device over SS port to xHCI and the device doesn't provide SS descriptors -
> the enumeration fails and it's ok. But if you connect the same device to a
> HS port - it should work properly. This is what I tried to simulate with
> this parameter.
it doesn't just fails, it gives the device over to the shared_hcd :-)
--
balbi
> > > > +
> > > > +static struct dummy_hcd_module_parameters mod_data = {
> > > > + .is_super_speed = false
> > > > +};
> > > > +module_param_named(is_super_speed, mod_data.is_super_speed,
> bool,
> > > > +S_IRUGO); MODULE_PARM_DESC(is_super_speed, "true to simulate
> > > > +SuperSpeed connection");
> > >
> > > you shouldn't need this. You should always enable SuperSpeed for
> > > this driver.
> >
> > You mean I don't need the module parameter? IMO it's the best way to
> > enable HS connection. If driver->speed=USB_SPEED_SUPER than dummy_hcd
> > will try to enumerate the device on the SS root hub and if the gadget
> > didn't provide SS descriptors - it will fail. Just as it happened
> > before. Finding out from
>
> then it should hand the device over to the hs_hcd ;-) Meaning it would
> disconnect the device, switch to hs_hcd and reconnect :-)
Yes this will be the best solution :) But as I said, the enumeration occurs
not in dummy_hcd thus I'm not sure how dummy_hcd can find out that it failed
in order to reconnect the device to hs_hcd. And I'm not sure that dummy_hcd
should care whether the enumeration succeeds or fails. It seems to me that
this should be handled by upper levels. Shouldn't it?
But we're discussing a more general issue; do you remember what the usb30
spec sais about this? I mean when connecting a HS device to a SS port (over
SS cable), should it still enumerate as a HS device? It seems to me that the
answer is no but I'm not sure...
>
> > dummy_hcd that the enumeration failed is very complicated (if even
> > possible) and I'm not sure that is the right thing to do... If you
> > connect a real device over SS port to xHCI and the device doesn't
> > provide SS descriptors - the enumeration fails and it's ok. But if
> you
> > connect the same device to a HS port - it should work properly. This
> > is what I tried to simulate with this parameter.
>
> it doesn't just fails, it gives the device over to the shared_hcd :-)
>
Hmmm.... I have v2.6.38 installed on my Linux-host at the moment and I just
verified that when connecting a Gadget driver without SS descriptors over SS
connection - the enumeration fails. Was this fixed in v2.6.39? It will take
me some time to upgrade to v2.6.39 and verify but I have to do it any way
so I'll give it a shot...
Best regards,
Tanya Brokhman
Consultant for Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
hi,
On Mon, May 23, 2011 at 11:18:53AM +0300, Tanya Brokhman wrote:
> > > > > +
> > > > > +static struct dummy_hcd_module_parameters mod_data = {
> > > > > + .is_super_speed = false
> > > > > +};
> > > > > +module_param_named(is_super_speed, mod_data.is_super_speed,
> > bool,
> > > > > +S_IRUGO); MODULE_PARM_DESC(is_super_speed, "true to simulate
> > > > > +SuperSpeed connection");
> > > >
> > > > you shouldn't need this. You should always enable SuperSpeed for
> > > > this driver.
> > >
> > > You mean I don't need the module parameter? IMO it's the best way to
> > > enable HS connection. If driver->speed=USB_SPEED_SUPER than dummy_hcd
> > > will try to enumerate the device on the SS root hub and if the gadget
> > > didn't provide SS descriptors - it will fail. Just as it happened
> > > before. Finding out from
> >
> > then it should hand the device over to the hs_hcd ;-) Meaning it would
> > disconnect the device, switch to hs_hcd and reconnect :-)
>
> Yes this will be the best solution :) But as I said, the enumeration occurs
> not in dummy_hcd thus I'm not sure how dummy_hcd can find out that it failed
take a look at xhci-ring.c for an example :-)
see that it check whether the attached device is a USB3.0 device or
USB2.0/1.1 device and chooses hcd or shared_hcd accordingly.
> in order to reconnect the device to hs_hcd. And I'm not sure that dummy_hcd
> should care whether the enumeration succeeds or fails. It seems to me that
> this should be handled by upper levels. Shouldn't it?
nope. If dummy_hcd was a SW xhci implementation then it should be taken
care by upper layers, but since this is a complete SW non-standard Host
interface, then we need to handle the whole thing.
> But we're discussing a more general issue; do you remember what the usb30
> spec sais about this? I mean when connecting a HS device to a SS port (over
not the exact wording of the Specs but it should work :-)
> SS cable), should it still enumerate as a HS device? It seems to me that the
> answer is no but I'm not sure...
the USB3 host shouldn't enumerate but it has the shared_hcd which is
USB2.0 compliant.
> > > dummy_hcd that the enumeration failed is very complicated (if even
> > > possible) and I'm not sure that is the right thing to do... If you
> > > connect a real device over SS port to xHCI and the device doesn't
> > > provide SS descriptors - the enumeration fails and it's ok. But if
> > you
> > > connect the same device to a HS port - it should work properly. This
> > > is what I tried to simulate with this parameter.
> >
> > it doesn't just fails, it gives the device over to the shared_hcd :-)
> >
>
> Hmmm.... I have v2.6.38 installed on my Linux-host at the moment and I just
> verified that when connecting a Gadget driver without SS descriptors over SS
> connection - the enumeration fails. Was this fixed in v2.6.39? It will take
> me some time to upgrade to v2.6.39 and verify but I have to do it any way
> so I'll give it a shot...
Maybe Sarah can give more details on this one. Sarah, what happens if we
attach USB2.0 device to USB3.0 roothub ?
--
balbi
Hi
> > > >
> > > > You mean I don't need the module parameter? IMO it's the best way
> > > > to enable HS connection. If driver->speed=USB_SPEED_SUPER than
> > > > dummy_hcd will try to enumerate the device on the SS root hub and
> > > > if the gadget didn't provide SS descriptors - it will fail. Just
> > > > as it happened before. Finding out from
> > >
> > > then it should hand the device over to the hs_hcd ;-) Meaning it
> > > would disconnect the device, switch to hs_hcd and reconnect :-)
> >
> > Yes this will be the best solution :) But as I said, the enumeration
> > occurs not in dummy_hcd thus I'm not sure how dummy_hcd can find out
> > that it failed
>
> take a look at xhci-ring.c for an example :-)
>
> see that it check whether the attached device is a USB3.0 device or
> USB2.0/1.1 device and chooses hcd or shared_hcd accordingly.
>
I ran some more tests with xhci and I think (or hope :) ) I figured this
out:
When connecting a gadget driver that is marked as SS device (the flag
CONFIG_USB_GADGET_IS_SUPER_SPEED = true) to a SS port over SS cable -
the enumeration fails if that gadget driver doesn't provide SS descriptors.
BUT: if I connect the same device via HS cable to SS port - the enumeration
is successful. I think that this is the case where xhci-ring handles the
device over to the HS hcd :) (By the way, I think that in xhci the
shared_hcd is SS and the main_hcd is HS)
In conclusion it seems to me that the device speed is determined by 2
things:
1. the cable used
2. whether the device HW supports SS protocol. In our scenario it can since
SS support is enabled in our udc. (We haven't released it yet.)
So when a HS device is connected to a SS port, the xHCI checks it's speed
and if necessary handles it over to the SS root hub. But this is done prior
to the enumeration phase so if the device speed is SS but it has no SS
descriptors - the enumeration will fail. The enumeration itself occurs not
in xhci but in hub.c so the xhci isn't aware of the fact that it failed and
doesn't handle this.
Since in dummy_hcd all of this is much simpler I think that the device speed
should be determined by driver->speed and "which type of cable the
connection was made over - SS or HS". The "cable type" is exactly what the
module parameter is.
My familiarity with the Linux host isn't as good as I would like it to be
(still working on that) so I might be wrong with my conclusions...
Best regards,
Tanya Brokhman
Consultant for Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
On Mon, 23 May 2011, Felipe Balbi wrote:
> > diff --git a/drivers/usb/gadget/dummy_hcd.c b/drivers/usb/gadget/dummy_hcd.c
> > index bf7981d..c2731d3 100644
> > --- a/drivers/usb/gadget/dummy_hcd.c
> > +++ b/drivers/usb/gadget/dummy_hcd.c
> > @@ -70,6 +70,15 @@ MODULE_DESCRIPTION (DRIVER_DESC);
> > MODULE_AUTHOR ("David Brownell");
> > MODULE_LICENSE ("GPL");
> >
> > +struct dummy_hcd_module_parameters {
> > + bool is_super_speed;
> > +};
> > +
> > +static struct dummy_hcd_module_parameters mod_data = {
> > + .is_super_speed = false
> > +};
> > +module_param_named(is_super_speed, mod_data.is_super_speed, bool, S_IRUGO);
> > +MODULE_PARM_DESC(is_super_speed, "true to simulate SuperSpeed connection");
>
> you shouldn't need this. You should always enable SuperSpeed for this
> driver.
I would say the opposite and go even farther. It's good to have a way
to force a SuperSpeed gadget to run at high speed, and it would also be
good to have a way to force a high-speed gadget to run at full speed.
A module parameter seems to be the simplest way of doing this.
> > + /*
> > + * We're connected and not reseted (reset occured now),
>
> 'reseted' isn't proper spelling :-)
Neither is "occured".
> > @@ -1371,6 +1577,10 @@ static void dummy_timer(unsigned long _dum_hcd)
> > case USB_SPEED_HIGH:
> > total = 512/*bytes*/ * 13/*packets*/ * 8/*uframes*/;
> > break;
> > + case USB_SPEED_SUPER:
> > + /* Bus speed is 500000 bytes/ms, so use a little less */
>
> isn't it 500000 bits/ms ?
SuperSpeed is 5 billion bits/s or 5 million bits/ms. The physical
layer encodes each data byte using 10 bits, therefore the bus speed is
500000 bytes/ms.
Alan Stern
On Mon, 23 May 2011, Felipe Balbi wrote:
> > > > +module_param_named(is_super_speed, mod_data.is_super_speed, bool,
> > > > +S_IRUGO); MODULE_PARM_DESC(is_super_speed, "true to simulate
> > > > +SuperSpeed connection");
> > >
> > > you shouldn't need this. You should always enable SuperSpeed for this
> > > driver.
> >
> > You mean I don't need the module parameter? IMO it's the best way to enable
> > HS connection. If driver->speed=USB_SPEED_SUPER than dummy_hcd will try to
> > enumerate the device on the SS root hub and if the gadget didn't provide SS
> > descriptors - it will fail. Just as it happened before. Finding out from
>
> then it should hand the device over to the hs_hcd ;-) Meaning it would
> disconnect the device, switch to hs_hcd and reconnect :-)
No. That doesn't happen with real devices, so there's no reason
dummy-hcd should do it.
> > dummy_hcd that the enumeration failed is very complicated (if even possible)
> > and I'm not sure that is the right thing to do... If you connect a real
> > device over SS port to xHCI and the device doesn't provide SS descriptors -
> > the enumeration fails and it's ok. But if you connect the same device to a
> > HS port - it should work properly. This is what I tried to simulate with
> > this parameter.
>
> it doesn't just fails, it gives the device over to the shared_hcd :-)
It does not. xhci-hcd does not keep track of whether or not
enumeration succeeds, and it doesn't do anything special when
enumeration fails.
Alan Stern
On Mon, 23 May 2011, Tanya Brokhman wrote:
> I ran some more tests with xhci and I think (or hope :) ) I figured this
> out:
> When connecting a gadget driver that is marked as SS device (the flag
> CONFIG_USB_GADGET_IS_SUPER_SPEED = true) to a SS port over SS cable -
> the enumeration fails if that gadget driver doesn't provide SS descriptors.
> BUT: if I connect the same device via HS cable to SS port - the enumeration
> is successful. I think that this is the case where xhci-ring handles the
> device over to the HS hcd :) (By the way, I think that in xhci the
> shared_hcd is SS and the main_hcd is HS)
>
> In conclusion it seems to me that the device speed is determined by 2
> things:
> 1. the cable used
> 2. whether the device HW supports SS protocol. In our scenario it can since
> SS support is enabled in our udc. (We haven't released it yet.)
> So when a HS device is connected to a SS port, the xHCI checks it's speed
> and if necessary handles it over to the SS root hub. But this is done prior
> to the enumeration phase so if the device speed is SS but it has no SS
> descriptors - the enumeration will fail. The enumeration itself occurs not
> in xhci but in hub.c so the xhci isn't aware of the fact that it failed and
> doesn't handle this.
>
> Since in dummy_hcd all of this is much simpler I think that the device speed
> should be determined by driver->speed and "which type of cable the
> connection was made over - SS or HS". The "cable type" is exactly what the
> module parameter is.
>
> My familiarity with the Linux host isn't as good as I would like it to be
> (still working on that) so I might be wrong with my conclusions...
Your analysis is basically correct.
Alan Stern
On Mon, May 23, 2011 at 11:32:18AM +0300, Felipe Balbi wrote:
> hi,
>
> On Mon, May 23, 2011 at 11:18:53AM +0300, Tanya Brokhman wrote:
> > > > > > +
> > > > > > +static struct dummy_hcd_module_parameters mod_data = {
> > > > > > + .is_super_speed = false
> > > > > > +};
> > > > > > +module_param_named(is_super_speed, mod_data.is_super_speed,
> > > bool,
> > > > > > +S_IRUGO); MODULE_PARM_DESC(is_super_speed, "true to simulate
> > > > > > +SuperSpeed connection");
> > > > >
> > > > > you shouldn't need this. You should always enable SuperSpeed for
> > > > > this driver.
> > > >
> > > > You mean I don't need the module parameter? IMO it's the best way to
> > > > enable HS connection. If driver->speed=USB_SPEED_SUPER than dummy_hcd
> > > > will try to enumerate the device on the SS root hub and if the gadget
> > > > didn't provide SS descriptors - it will fail. Just as it happened
> > > > before. Finding out from
> > >
> > > then it should hand the device over to the hs_hcd ;-) Meaning it would
> > > disconnect the device, switch to hs_hcd and reconnect :-)
> >
> > Yes this will be the best solution :) But as I said, the enumeration occurs
> > not in dummy_hcd thus I'm not sure how dummy_hcd can find out that it failed
>
> take a look at xhci-ring.c for an example :-)
>
> see that it check whether the attached device is a USB3.0 device or
> USB2.0/1.1 device and chooses hcd or shared_hcd accordingly.
Yes, that's based on what the port speed registers report. The host
controller will detect either a connect on the SuperSpeed terminations
and set the SuperSpeed speed accordingly. I'm not sure how it detects
whether the device is a high speed device. I think it has something to
do with the LPF signaling, but I'm not a link layer expert.
> > in order to reconnect the device to hs_hcd. And I'm not sure that dummy_hcd
> > should care whether the enumeration succeeds or fails. It seems to me that
> > this should be handled by upper levels. Shouldn't it?
>
> nope. If dummy_hcd was a SW xhci implementation then it should be taken
> care by upper layers, but since this is a complete SW non-standard Host
> interface, then we need to handle the whole thing.
>
> > But we're discussing a more general issue; do you remember what the usb30
> > spec sais about this? I mean when connecting a HS device to a SS port (over
>
> not the exact wording of the Specs but it should work :-)
>
> > SS cable), should it still enumerate as a HS device? It seems to me that the
> > answer is no but I'm not sure...
If you look at Figure 7-13 in the USB 3.0 spec, you'll see that the
device is supposed to try to detect SuperSpeed terminations, but if that
times out, then it goes to the "SuperSpeed disabled" state, and falls
back to the USB 2.0 bus connection.
> the USB3 host shouldn't enumerate but it has the shared_hcd which is
> USB2.0 compliant.
Felipe, technically the xHCI host controller handles all device speeds.
There are no companion controllers in an xHCI host controller. The hcd
and shared_hcd is just an abstraction to make the roothub look like an
external USB 3.0 hub to the USB core.
External USB 3.0 devices show up as two separate devices -- a USB 3.0
hub and a USB 2.0 hub. The xHCI host controller originally just showed
as a USB 3.0 hub that could handle all speeds, but when we needed to add
support for external USB 3.0 hubs, that simplification broke the USB
core's assumption that roothubs and external hubs act the same way. So
we changed the xHCI driver to register to usb_hcd structures for one PCI
device.
Basically you should just think of the xHCI host as taking care of all
devices plugged into it.
> > > > dummy_hcd that the enumeration failed is very complicated (if even
> > > > possible) and I'm not sure that is the right thing to do... If you
> > > > connect a real device over SS port to xHCI and the device doesn't
> > > > provide SS descriptors - the enumeration fails and it's ok. But if
> > > you
> > > > connect the same device to a HS port - it should work properly. This
> > > > is what I tried to simulate with this parameter.
> > >
> > > it doesn't just fails, it gives the device over to the shared_hcd :-)
> > >
> >
> > Hmmm.... I have v2.6.38 installed on my Linux-host at the moment and I just
> > verified that when connecting a Gadget driver without SS descriptors over SS
> > connection - the enumeration fails. Was this fixed in v2.6.39? It will take
> > me some time to upgrade to v2.6.39 and verify but I have to do it any way
> > so I'll give it a shot...
>
> Maybe Sarah can give more details on this one. Sarah, what happens if we
> attach USB2.0 device to USB3.0 roothub ?
It gets enumerated under xHCI as a USB 2.0 device. With 2.6.39 and
later, you'll see it ends up under the "Linux Foundation 2.0 root hub",
but if you look at iManufacturer under that hub's description, you'll
see it's an xHCI roothub. With kernels before 2.6.39, it will end up
under the "Linux Foundation 3.0 root hub".
Sarah Sharp
On Mon, May 23, 2011 at 12:16:40PM +0300, Tanya Brokhman wrote:
> Hi
>
> > > > >
> > > > > You mean I don't need the module parameter? IMO it's the best way
> > > > > to enable HS connection. If driver->speed=USB_SPEED_SUPER than
> > > > > dummy_hcd will try to enumerate the device on the SS root hub and
> > > > > if the gadget didn't provide SS descriptors - it will fail. Just
> > > > > as it happened before. Finding out from
> > > >
> > > > then it should hand the device over to the hs_hcd ;-) Meaning it
> > > > would disconnect the device, switch to hs_hcd and reconnect :-)
> > >
> > > Yes this will be the best solution :) But as I said, the enumeration
> > > occurs not in dummy_hcd thus I'm not sure how dummy_hcd can find out
> > > that it failed
> >
> > take a look at xhci-ring.c for an example :-)
> >
> > see that it check whether the attached device is a USB3.0 device or
> > USB2.0/1.1 device and chooses hcd or shared_hcd accordingly.
> >
>
> I ran some more tests with xhci and I think (or hope :) ) I figured this
> out:
> When connecting a gadget driver that is marked as SS device (the flag
> CONFIG_USB_GADGET_IS_SUPER_SPEED = true) to a SS port over SS cable -
> the enumeration fails if that gadget driver doesn't provide SS descriptors.
If you're connecting as a SuperSpeed device, you need to have SuperSpeed
descriptors. You definitely won't pass the USB-IF compliance test if
you don't have those descriptors. :) You need to be able to
automatically fall back to high speed with high speed descriptors when
necessary.
> BUT: if I connect the same device via HS cable to SS port - the enumeration
> is successful. I think that this is the case where xhci-ring handles the
> device over to the HS hcd :) (By the way, I think that in xhci the
> shared_hcd is SS and the main_hcd is HS)
Yes, main_hcd is the high speed portion of the roothub. It's registered
first, so that during a system resume, the HS roothub gets the devices
reset first, and any USB 3.0 devices that had erroneously connected on
USB 2.0 can migrate over to the SuperSpeed terminations. (If you are a
USB 2.0 device that also has the ability to work at USB 3.0, you don't
change speeds after USB 2.0 connection unless you get a reset.) We had
to register the USB 2.0 roothub first so that USB 2.0 devices would get
reset first, otherwise persistent storage might fail for USB 3.0
devices.
> In conclusion it seems to me that the device speed is determined by 2
> things:
> 1. the cable used
Not necessarily the cable used. You can have a SuperSpeed cable and
have both host and device be SuperSpeed capable, but signal noise could
cause the SuperSpeed enumeration to fail.
> 2. whether the device HW supports SS protocol. In our scenario it can since
> SS support is enabled in our udc. (We haven't released it yet.)
What is a UDC?
> So when a HS device is connected to a SS port, the xHCI checks it's speed
> and if necessary handles it over to the SS root hub. But this is done prior
> to the enumeration phase so if the device speed is SS but it has no SS
> descriptors - the enumeration will fail. The enumeration itself occurs not
> in xhci but in hub.c so the xhci isn't aware of the fact that it failed and
> doesn't handle this.
The USB core might try to disable the device in that case, and the xHCI
driver would see that.
> Since in dummy_hcd all of this is much simpler I think that the device speed
> should be determined by driver->speed and "which type of cable the
> connection was made over - SS or HS". The "cable type" is exactly what the
> module parameter is.
I really don't understand this. You're going to have a module parameter
for what type of cable is plugged in? How can you tell which one the
user is going to use? What about the case where SuperSpeed enumeration
fails and you have to fall back to high speed? It seems like you really
need to handle both speeds and the speed fall back parameter in the same
driver. Isn't there some other gadget driver that has a fall back to
full or low speed when high speed enumeration fails?
Sarah Sharp
On Mon, 23 May 2011, Sarah Sharp wrote:
> > 2. whether the device HW supports SS protocol. In our scenario it can since
> > SS support is enabled in our udc. (We haven't released it yet.)
>
> What is a UDC?
USB Device Controller. It's the device-side analog of a host
controller.
> > Since in dummy_hcd all of this is much simpler I think that the device speed
> > should be determined by driver->speed and "which type of cable the
> > connection was made over - SS or HS". The "cable type" is exactly what the
> > module parameter is.
>
> I really don't understand this. You're going to have a module parameter
> for what type of cable is plugged in?
It would be more accurate to say the module parameter will be used to
force the connection to run at a lower speed than the maximum possible.
This is kind of like what happens when you plug in a SuperSpeed device
using a USB-2 cable -- the connection runs at a lower speed than it
could have.
> How can you tell which one the
> user is going to use?
This isn't about actual cables or devices. dummy-hcd is an emulator;
it presents as a host controller and a device controller both on the
same system. This allows people to test gadget drivers without having
any UDC hardware.
> What about the case where SuperSpeed enumeration
> fails and you have to fall back to high speed?
If SuperSpeed enumeration fails, say because the device doesn't have
any SuperSpeed descriptors, xhci-hcd doesn't fall back to high speed,
does it? dummy-hcd should behave the same way.
> It seems like you really
> need to handle both speeds and the speed fall back parameter in the same
> driver. Isn't there some other gadget driver that has a fall back to
> full or low speed when high speed enumeration fails?
That's a property of the gadget driver, not the UDC driver. dummy-hcd
is a UDC driver (and an HCD too).
Alan Stern
Hi,
(replying from another PC, not sure if formatting
will be broken, hopefully not)
On May 23, 2011, at 7:28 PM, Alan Stern wrote:
>>> Since in dummy_hcd all of this is much simpler I think that the device speed
>>> should be determined by driver->speed and "which type of cable the
>>> connection was made over - SS or HS". The "cable type" is exactly what the
>>> module parameter is.
>>
>> I really don't understand this. You're going to have a module parameter
>> for what type of cable is plugged in?
>
> It would be more accurate to say the module parameter will be used to
> force the connection to run at a lower speed than the maximum possible.
> This is kind of like what happens when you plug in a SuperSpeed device
> using a USB-2 cable -- the connection runs at a lower speed than it
> could have.
if it's something like that, for sure we can have the module parameter. But
plugging a USB2 gadget/function to a USB3-capable UDC/HCD should
work fine.
With Tatyana's patches, if we load a USB2 g_zero to dummy_hcd, enumeration
will fail where it shouldn't. This has been my whole point ;-) Maybe I wasn't
clear enough.
>> What about the case where SuperSpeed enumeration
>> fails and you have to fall back to high speed?
>
> If SuperSpeed enumeration fails, say because the device doesn't have
> any SuperSpeed descriptors, xhci-hcd doesn't fall back to high speed,
> does it? dummy-hcd should behave the same way.
it should at least. Isn't that what happens between EHCI/OHCI ? HS Chirp
sequencing fails, then we fall back to FullSpeed.
>> It seems like you really
>> need to handle both speeds and the speed fall back parameter in the same
>> driver. Isn't there some other gadget driver that has a fall back to
>> full or low speed when high speed enumeration fails?
>
> That's a property of the gadget driver, not the UDC driver. dummy-hcd
> is a UDC driver (and an HCD too).
USB3.0 dummy_hcd should still enumerate USB2.0 gadget drivers.
--
balbi
On Tue, 24 May 2011, Felipe Balbi wrote:
> > It would be more accurate to say the module parameter will be used to
> > force the connection to run at a lower speed than the maximum possible.
> > This is kind of like what happens when you plug in a SuperSpeed device
> > using a USB-2 cable -- the connection runs at a lower speed than it
> > could have.
>
> if it's something like that, for sure we can have the module parameter. But
> plugging a USB2 gadget/function to a USB3-capable UDC/HCD should
> work fine.
>
> With Tatyana's patches, if we load a USB2 g_zero to dummy_hcd, enumeration
> will fail where it shouldn't. This has been my whole point ;-) Maybe I wasn't
> clear enough.
I guess not. I thought Tatyana said she was working to fix that bug...
> >> What about the case where SuperSpeed enumeration
> >> fails and you have to fall back to high speed?
> >
> > If SuperSpeed enumeration fails, say because the device doesn't have
> > any SuperSpeed descriptors, xhci-hcd doesn't fall back to high speed,
> > does it? dummy-hcd should behave the same way.
>
> it should at least. Isn't that what happens between EHCI/OHCI ? HS Chirp
> sequencing fails, then we fall back to FullSpeed.
That's a failure in initialization, not a failure in enumeration.
There are two reasons why the HS chirp might fail: the device doesn't
support high speed operation or hardware errors prevent the chirp from
working. With dummy-hcd there are no hardware errors (because there's
no hardware).
As for whether or not the device supports high-speed or SuperSpeed
operation, that's determined by the usb_gadget_driver->speed field. If
the field doesn't specify SuperSpeed then dummy-hcd should connect the
gadget to the USB-2 root hub rather than the USB-3 root hub. Isn't
that what Tatyana's patch does? It contains a line saying:
dum->gadget.speed = driver->speed;
> >> It seems like you really
> >> need to handle both speeds and the speed fall back parameter in the same
> >> driver. Isn't there some other gadget driver that has a fall back to
> >> full or low speed when high speed enumeration fails?
> >
> > That's a property of the gadget driver, not the UDC driver. dummy-hcd
> > is a UDC driver (and an HCD too).
>
> USB3.0 dummy_hcd should still enumerate USB2.0 gadget drivers.
Yes, certainly it should. If it doesn't, that's a bug, not a design
error.
Alan Stern
> >
> > With Tatyana's patches, if we load a USB2 g_zero to dummy_hcd,
> enumeration
> > will fail where it shouldn't. This has been my whole point ;-) Maybe
> I wasn't
> > clear enough.
>
> I guess not. I thought Tatyana said she was working to fix that bug...
This bug is fixed with the module parameter. When dummy_hcd is loaded it
registers 2 root hubs: HS & SS but unless the is_super_speed parameter is
true, the device will be connected to the HS hub. This way when loading HS
g_zero it enumerates under HS root hub successfully.
Note that if load SS gadget without setting is_super_speed=true for dummy
hcd, it will also enumerate under HS hub! This is basically a way to test
that SS devices are backward compatible.
> > >> What about the case where SuperSpeed enumeration
> > >> fails and you have to fall back to high speed?
> > >
> > > If SuperSpeed enumeration fails, say because the device doesn't
> have
> > > any SuperSpeed descriptors, xhci-hcd doesn't fall back to high
> speed,
> > > does it? dummy-hcd should behave the same way.
> >
> > it should at least. Isn't that what happens between EHCI/OHCI ? HS
> Chirp
> > sequencing fails, then we fall back to FullSpeed.
>
> That's a failure in initialization, not a failure in enumeration.
>
> There are two reasons why the HS chirp might fail: the device doesn't
> support high speed operation or hardware errors prevent the chirp from
> working. With dummy-hcd there are no hardware errors (because there's
> no hardware).
>
> As for whether or not the device supports high-speed or SuperSpeed
> operation, that's determined by the usb_gadget_driver->speed field. If
> the field doesn't specify SuperSpeed then dummy-hcd should connect the
> gadget to the USB-2 root hub rather than the USB-3 root hub. Isn't
> that what Tatyana's patch does? It contains a line saying:
>
> dum->gadget.speed = driver->speed;
You're right. This is exactly what is done with a small update: if
is_super_speed=false the gadget will connect to a USB2 root hub even if
driver->speed=USB_SPEED_SUPER. In that case dum->gadget.speed will be set to
USB_SPEED_HIGH. The code that sets this now is:
if (mod_data.is_super_speed)
dum->gadget.speed = driver->speed;
else
dum->gadget.speed = min((u8)USB_SPEED_HIGH,
(u8)driver->speed);
if (dum->gadget.speed < driver->speed)
dev_dbg(udc_dev(dum), "This device can perform faster if"
" you connect it to a "
"SupeSpeed port...\n");
> > >> It seems like you really
> > >> need to handle both speeds and the speed fall back parameter in
> the same
> > >> driver. Isn't there some other gadget driver that has a fall back
> to
> > >> full or low speed when high speed enumeration fails?
> > >
> > > That's a property of the gadget driver, not the UDC driver. dummy-
> hcd
> > > is a UDC driver (and an HCD too).
> >
> > USB3.0 dummy_hcd should still enumerate USB2.0 gadget drivers.
>
> Yes, certainly it should. If it doesn't, that's a bug, not a design
> error.
>
This version of dummy_hcd enumerates HS devices without any issues.
Best regards,
Tanya Brokhman
Consultant for Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
Hi,
On Tue, May 24, 2011 at 08:53:15AM +0300, Tanya Brokhman wrote:
> > >
> > > With Tatyana's patches, if we load a USB2 g_zero to dummy_hcd,
> > enumeration
> > > will fail where it shouldn't. This has been my whole point ;-) Maybe
> > I wasn't
> > > clear enough.
> >
> > I guess not. I thought Tatyana said she was working to fix that bug...
>
> This bug is fixed with the module parameter. When dummy_hcd is loaded it
> registers 2 root hubs: HS & SS but unless the is_super_speed parameter is
> true, the device will be connected to the HS hub. This way when loading HS
> g_zero it enumerates under HS root hub successfully.
> Note that if load SS gadget without setting is_super_speed=true for dummy
> hcd, it will also enumerate under HS hub! This is basically a way to test
> that SS devices are backward compatible.
but you also need to make sure that HS gadget enumerates on SS host. I
don't agree that the module parameter is the only option.
> > > >> What about the case where SuperSpeed enumeration
> > > >> fails and you have to fall back to high speed?
> > > >
> > > > If SuperSpeed enumeration fails, say because the device doesn't
> > have
> > > > any SuperSpeed descriptors, xhci-hcd doesn't fall back to high
> > speed,
> > > > does it? dummy-hcd should behave the same way.
> > >
> > > it should at least. Isn't that what happens between EHCI/OHCI ? HS
> > Chirp
> > > sequencing fails, then we fall back to FullSpeed.
> >
> > That's a failure in initialization, not a failure in enumeration.
> >
> > There are two reasons why the HS chirp might fail: the device doesn't
> > support high speed operation or hardware errors prevent the chirp from
> > working. With dummy-hcd there are no hardware errors (because there's
> > no hardware).
> >
> > As for whether or not the device supports high-speed or SuperSpeed
> > operation, that's determined by the usb_gadget_driver->speed field. If
> > the field doesn't specify SuperSpeed then dummy-hcd should connect the
> > gadget to the USB-2 root hub rather than the USB-3 root hub. Isn't
> > that what Tatyana's patch does? It contains a line saying:
> >
> > dum->gadget.speed = driver->speed;
>
> You're right. This is exactly what is done with a small update: if
> is_super_speed=false the gadget will connect to a USB2 root hub even if
> driver->speed=USB_SPEED_SUPER. In that case dum->gadget.speed will be set to
> USB_SPEED_HIGH. The code that sets this now is:
> if (mod_data.is_super_speed)
> dum->gadget.speed = driver->speed;
> else
> dum->gadget.speed = min((u8)USB_SPEED_HIGH,
ok ok, I took a closer look at the original patches and you're changing
drivers->speed to super before converting the gadget drivers to
superspeed. That's what's making enumeration fail on all those guys.
> > > >> It seems like you really
> > > >> need to handle both speeds and the speed fall back parameter in
> > the same
> > > >> driver. Isn't there some other gadget driver that has a fall back
> > to
> > > >> full or low speed when high speed enumeration fails?
> > > >
> > > > That's a property of the gadget driver, not the UDC driver. dummy-
> > hcd
> > > > is a UDC driver (and an HCD too).
> > >
> > > USB3.0 dummy_hcd should still enumerate USB2.0 gadget drivers.
> >
> > Yes, certainly it should. If it doesn't, that's a bug, not a design
> > error.
> >
>
> This version of dummy_hcd enumerates HS devices without any issues.
that's not what I saw, but then again, maybe you changed driver->speed
to super and didn't provide superspeed descriptors.
--
balbi
Hi Felipe
> >
> > This version of dummy_hcd enumerates HS devices without any issues.
>
> that's not what I saw,
By "this version" I mean v12 and the one I just emailed, v13. Did you try
them out? I did and all worked just fine...
> but then again, maybe you changed driver->speed
> to super and didn't provide superspeed descriptors.
>
Yes :) The driver->speed is updated in usb_composite_probe() if
CONFIG_USB_GADGET_SUPERSPEED is defined.
So, are we ok with this solution? The module parameter I mean?
Are you going to try the v13 in your branch? Please let me know how it goes
and of course if you have any comments.
Thanks!
Tanya Brokhman
Consultant for Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
Hi,
On Tue, May 24, 2011 at 01:31:14PM +0300, Tanya Brokhman wrote:
> > but then again, maybe you changed driver->speed
> > to super and didn't provide superspeed descriptors.
> >
>
> Yes :) The driver->speed is updated in usb_composite_probe() if
> CONFIG_USB_GADGET_SUPERSPEED is defined.
>
> So, are we ok with this solution? The module parameter I mean?
> Are you going to try the v13 in your branch? Please let me know how it goes
> and of course if you have any comments.
I think it still gives the possibility for failure. I would rather not
take that until all gadget drivers are fixed. We can help you doing that
and we only change driver->speed after all gadget drivers have their
"sensible defaults" SuperSpeed descriptors.
What do you say ? Other than that, the module parameter is, like Alan
Said, useful for debugging, so we might as well keep it.
--
balbi
Hi Felipe
> > Yes :) The driver->speed is updated in usb_composite_probe() if
> > CONFIG_USB_GADGET_SUPERSPEED is defined.
> >
> > So, are we ok with this solution? The module parameter I mean?
> > Are you going to try the v13 in your branch? Please let me know how
> it
> > goes and of course if you have any comments.
>
> I think it still gives the possibility for failure. I would rather not
> take that until all gadget drivers are fixed. We can help you doing
> that and we only change driver->speed after all gadget drivers have
> their "sensible defaults" SuperSpeed descriptors.
By "until all gadget drivers are fixed" you mean until all gadget drivers
provide SS descriptors? This will take for ever...
I wasn't about to modify all gadget drivers and to add SS descriptors for
them. I can add default values (as generate_ss_descriptors() did if you
remember) but I don't think this is the right approach because as you said -
different gadget drivers might have different SS descriptors and I don't
feel confident enough to set these values. Nor do I have the ability to test
each of the gadget drivers the way I would like to after this change.
The only gadget driver I felt confident adding SS descriptors for is UASP,
which I tested properly.
Actually if the CONFIG_USB_GADGET_SUPERSPEED is turned off, which is the
default of it, the speed won't be updated and all these series won't be
functional so I don't see any possibilities for failure in such
configuration. Or am I missing something?
Best regards,
Tanya Brokhman
Consultant for Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
On Tue, 24 May 2011, Tanya Brokhman wrote:
> Hi Felipe
>
> > > Yes :) The driver->speed is updated in usb_composite_probe() if
> > > CONFIG_USB_GADGET_SUPERSPEED is defined.
> > >
> > > So, are we ok with this solution? The module parameter I mean?
> > > Are you going to try the v13 in your branch? Please let me know how
> > it
> > > goes and of course if you have any comments.
> >
> > I think it still gives the possibility for failure. I would rather not
> > take that until all gadget drivers are fixed. We can help you doing
> > that and we only change driver->speed after all gadget drivers have
> > their "sensible defaults" SuperSpeed descriptors.
>
> By "until all gadget drivers are fixed" you mean until all gadget drivers
> provide SS descriptors? This will take for ever...
> I wasn't about to modify all gadget drivers and to add SS descriptors for
> them. I can add default values (as generate_ss_descriptors() did if you
> remember) but I don't think this is the right approach because as you said -
> different gadget drivers might have different SS descriptors and I don't
> feel confident enough to set these values. Nor do I have the ability to test
> each of the gadget drivers the way I would like to after this change.
> The only gadget driver I felt confident adding SS descriptors for is UASP,
> which I tested properly.
>
> Actually if the CONFIG_USB_GADGET_SUPERSPEED is turned off, which is the
> default of it, the speed won't be updated and all these series won't be
> functional so I don't see any possibilities for failure in such
> configuration. Or am I missing something?
dummy_hcd should work when CONFIG_USB_GADGET_SUPERSPEED is enabled,
even if the usb_gadget_driver structure is initialized with the speed
field set to USB_SPEED_HIGH. This will be true for all the standalone
gadget drivers until they are updated.
Which leaves a question about the composite gadget framework. Should
it be updated with SS support? Probably not until the various function
drivers have all been updated.
Alan Stern
> >
> > Actually if the CONFIG_USB_GADGET_SUPERSPEED is turned off, which is
> the
> > default of it, the speed won't be updated and all these series won't
> be
> > functional so I don't see any possibilities for failure in such
> > configuration. Or am I missing something?
>
> dummy_hcd should work when CONFIG_USB_GADGET_SUPERSPEED is enabled,
> even if the usb_gadget_driver structure is initialized with the speed
> field set to USB_SPEED_HIGH. This will be true for all the standalone
> gadget drivers until they are updated.
It does.
>
> Which leaves a question about the composite gadget framework. Should
> it be updated with SS support? Probably not until the various function
> drivers have all been updated.
As I mentioned, updating all of the gadget drivers will take a long time and
I don't fill confident enough doing since I'm not familiar with all of them
and don't have the ability to test each of them properly. I can add SS
descriptors to f_mass_storage, g_zero if it helps and of course f_uasp
already has them.
I'm a bit confused by this actually... We've been discussing this patch
series for quite a while now and I got the impression that except for some
minor comments you were all for excepting this. Was I wrong or am I
misunderstanding the above?
In any case, I don't feel that adding SS support for the Gadget framework
should be delayed until all gadget drivers add SS descriptors because this
patch series will give the developers the ability to test these gadget
drivers at SS. Also, several developers addressed me offline with questions
on this series so I know people are using it in their work. And of course we
do :)
Best regards,
Tanya Brokhman
Consultant for Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
Hi,
On Tue, May 24, 2011 at 10:20:37AM -0400, Alan Stern wrote:
> On Tue, 24 May 2011, Tanya Brokhman wrote:
>
> > Hi Felipe
> >
> > > > Yes :) The driver->speed is updated in usb_composite_probe() if
> > > > CONFIG_USB_GADGET_SUPERSPEED is defined.
> > > >
> > > > So, are we ok with this solution? The module parameter I mean?
> > > > Are you going to try the v13 in your branch? Please let me know how
> > > it
> > > > goes and of course if you have any comments.
> > >
> > > I think it still gives the possibility for failure. I would rather not
> > > take that until all gadget drivers are fixed. We can help you doing
> > > that and we only change driver->speed after all gadget drivers have
> > > their "sensible defaults" SuperSpeed descriptors.
> >
> > By "until all gadget drivers are fixed" you mean until all gadget drivers
> > provide SS descriptors? This will take for ever...
> > I wasn't about to modify all gadget drivers and to add SS descriptors for
> > them. I can add default values (as generate_ss_descriptors() did if you
> > remember) but I don't think this is the right approach because as you said -
> > different gadget drivers might have different SS descriptors and I don't
> > feel confident enough to set these values. Nor do I have the ability to test
> > each of the gadget drivers the way I would like to after this change.
> > The only gadget driver I felt confident adding SS descriptors for is UASP,
> > which I tested properly.
> >
> > Actually if the CONFIG_USB_GADGET_SUPERSPEED is turned off, which is the
> > default of it, the speed won't be updated and all these series won't be
> > functional so I don't see any possibilities for failure in such
> > configuration. Or am I missing something?
>
> dummy_hcd should work when CONFIG_USB_GADGET_SUPERSPEED is enabled,
> even if the usb_gadget_driver structure is initialized with the speed
> field set to USB_SPEED_HIGH. This will be true for all the standalone
> gadget drivers until they are updated.
>
> Which leaves a question about the composite gadget framework. Should
> it be updated with SS support? Probably not until the various function
> drivers have all been updated.
We can add support for all the USB3 standard requests, just don't change
the driver->speed field until all gadget drivers are fixed.
--
balbi
Hi,
On Wed, May 25, 2011 at 07:46:38AM +0300, Tanya Brokhman wrote:
> > Which leaves a question about the composite gadget framework. Should
> > it be updated with SS support? Probably not until the various function
> > drivers have all been updated.
>
> As I mentioned, updating all of the gadget drivers will take a long time and
> I don't fill confident enough doing since I'm not familiar with all of them
> and don't have the ability to test each of them properly. I can add SS
> descriptors to f_mass_storage, g_zero if it helps and of course f_uasp
> already has them.
> I'm a bit confused by this actually... We've been discussing this patch
> series for quite a while now and I got the impression that except for some
> minor comments you were all for excepting this. Was I wrong or am I
> misunderstanding the above?
> In any case, I don't feel that adding SS support for the Gadget framework
> should be delayed until all gadget drivers add SS descriptors because this
> patch series will give the developers the ability to test these gadget
> drivers at SS. Also, several developers addressed me offline with questions
> on this series so I know people are using it in their work. And of course we
> do :)
just remove the hunk which changes composite.c speed field and it should
all be ok :-)
--
balbi
> > As I mentioned, updating all of the gadget drivers will take a long
> > time and I don't fill confident enough doing since I'm not familiar
> > with all of them and don't have the ability to test each of them
> > properly. I can add SS descriptors to f_mass_storage, g_zero if it
> > helps and of course f_uasp already has them.
> > I'm a bit confused by this actually... We've been discussing this
> > patch series for quite a while now and I got the impression that
> > except for some minor comments you were all for excepting this. Was I
> > wrong or am I misunderstanding the above?
> > In any case, I don't feel that adding SS support for the Gadget
> > framework should be delayed until all gadget drivers add SS
> > descriptors because this patch series will give the developers the
> > ability to test these gadget drivers at SS. Also, several developers
> > addressed me offline with questions on this series so I know people
> > are using it in their work. And of course we do :)
>
> just remove the hunk which changes composite.c speed field and it
> should all be ok :-)
>
But that's why we added the feature flag. Isn't leaving it FALSE the same as
removing the part that updates gadget speed? It is protected with #ifdef.
Best regards,
Tanya Brokhman
Consultant for Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
On Wed, May 25, 2011 at 12:26:08PM +0300, Tanya Brokhman wrote:
> > > As I mentioned, updating all of the gadget drivers will take a long
> > > time and I don't fill confident enough doing since I'm not familiar
> > > with all of them and don't have the ability to test each of them
> > > properly. I can add SS descriptors to f_mass_storage, g_zero if it
> > > helps and of course f_uasp already has them.
> > > I'm a bit confused by this actually... We've been discussing this
> > > patch series for quite a while now and I got the impression that
> > > except for some minor comments you were all for excepting this. Was I
> > > wrong or am I misunderstanding the above?
> > > In any case, I don't feel that adding SS support for the Gadget
> > > framework should be delayed until all gadget drivers add SS
> > > descriptors because this patch series will give the developers the
> > > ability to test these gadget drivers at SS. Also, several developers
> > > addressed me offline with questions on this series so I know people
> > > are using it in their work. And of course we do :)
> >
> > just remove the hunk which changes composite.c speed field and it
> > should all be ok :-)
> >
>
> But that's why we added the feature flag. Isn't leaving it FALSE the same as
> removing the part that updates gadget speed? It is protected with #ifdef.
you mean the module parameter ? This will mean that users will have to
remember another module parameter otherwise it will not work in some
situations :-)
the module should work without that parameter :-)
--
balbi
> > But that's why we added the feature flag. Isn't leaving it FALSE the
> > same as removing the part that updates gadget speed? It is protected
> with #ifdef.
>
> you mean the module parameter ? This will mean that users will have to
> remember another module parameter otherwise it will not work in some
> situations :-)
>
> the module should work without that parameter :-)
>
No, I'm afraid we're talking about different things here.
As far as dummy_hcd is concerned:
Dummy_hcd has a new module parameter that should be set to true if the
developer wishes the gadget driver to be connected to a SS root hub. Since
at the moment most of the gadget drivers woun't work at SS connection (lack
of SS descriptors) the default of this parameter is true and thus the
connected gadget driver will be enumerated over a HS root hub. Users don't
have to remember t set anything in order to be able to work with dummy_hcd
and the existing gadget drivers just as they did up until now.
We did add a new feature flag (CONFIG_USB_GADGET_SUPERSPEED) that is set
during compilation. If that flag is defined, then (and only then!) will the
driver->speed be set to SS by composite_bind. Please keep in mind the
following:
1. if CONFIG_USB_GADGET_SUPERSPEED=true, existing gadget drivers are still
functional with dummy_hcd since as I already mentioned, they will be
enumerated through HS root hub and thus the gadget.speed will be set to HS.
This is true for all gadget drivers, including the once that don't define SS
descriptors.
2. if CONFIG_USB_GADGET_SUPERSPEED=false (which is the default) then all the
code that is added by this patch series will never be executed since
driver->speed won't be updated (and will remain HS). This is basically a way
to disable SS support in the gadget framework.
Hope this clears the confusion :)
Best regards,
Tanya Brokhman
Consultant for Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
Hi,
On Wed, May 25, 2011 at 12:43:10PM +0300, Tanya Brokhman wrote:
> > > But that's why we added the feature flag. Isn't leaving it FALSE the
> > > same as removing the part that updates gadget speed? It is protected
> > with #ifdef.
> >
> > you mean the module parameter ? This will mean that users will have to
> > remember another module parameter otherwise it will not work in some
> > situations :-)
> >
> > the module should work without that parameter :-)
> >
>
> No, I'm afraid we're talking about different things here.
> As far as dummy_hcd is concerned:
> Dummy_hcd has a new module parameter that should be set to true if the
> developer wishes the gadget driver to be connected to a SS root hub. Since
> at the moment most of the gadget drivers woun't work at SS connection (lack
> of SS descriptors) the default of this parameter is true and thus the
> connected gadget driver will be enumerated over a HS root hub. Users don't
> have to remember t set anything in order to be able to work with dummy_hcd
> and the existing gadget drivers just as they did up until now.
>
> We did add a new feature flag (CONFIG_USB_GADGET_SUPERSPEED) that is set
> during compilation. If that flag is defined, then (and only then!) will the
> driver->speed be set to SS by composite_bind. Please keep in mind the
and that's the only thing I'm asking you to remove :-)
we should be to compile dummy_hcd in SuperSpeed and still have
high-speed gadget drivers :-)
> following:
> 1. if CONFIG_USB_GADGET_SUPERSPEED=true, existing gadget drivers are still
> functional with dummy_hcd since as I already mentioned, they will be
> enumerated through HS root hub and thus the gadget.speed will be set to HS.
> This is true for all gadget drivers, including the once that don't define SS
> descriptors.
only due the module parameter, right ?
--
balbi
Hi Felipe,
> >
> > No, I'm afraid we're talking about different things here.
> > As far as dummy_hcd is concerned:
> > Dummy_hcd has a new module parameter that should be set to true if
> the
> > developer wishes the gadget driver to be connected to a SS root hub.
> > Since at the moment most of the gadget drivers woun't work at SS
> > connection (lack of SS descriptors) the default of this parameter is
> > true and thus the connected gadget driver will be enumerated over a
> HS
> > root hub. Users don't have to remember t set anything in order to be
> > able to work with dummy_hcd and the existing gadget drivers just as
> they did up until now.
> >
> > We did add a new feature flag (CONFIG_USB_GADGET_SUPERSPEED) that is
> > set during compilation. If that flag is defined, then (and only
> then!)
> > will the
> > driver->speed be set to SS by composite_bind. Please keep in mind the
>
> and that's the only thing I'm asking you to remove :-)
You want to remove the following change from in composite.c:
@@ -1386,6 +1604,9 @@ int usb_composite_probe(struct usb_composite_driver
*driver,
driver->iProduct = driver->name;
composite_driver.function = (char *) driver->name;
composite_driver.driver.name = driver->name;
+#ifdef CONFIG_USB_GADGET_SUPERSPEED
+ composite_driver.speed = USB_SPEED_SUPER; #endif /*
+CONFIG_USB_GADGET_SUPERSPEED */
composite = driver;
composite_gadget_bind = bind;
Right? I'm sorry, but I really don't understand why... :( Removing it is the
same as not defining CONFIG_USB_GADGET_SUPERSPEED.
And if we do remove it: suppose you do have a SS gadget driver you wish to
test now with dummy_hcd. For example the f_uasp we released. In order to be
able to do so, you'll have to add the above manually in your repository.
Otherwise even if the dummy_hcd module parameter is_super_speed=true - the
gadget will enumerate over HS root hub since the driver speed will remain
HS.
>
> we should be to compile dummy_hcd in SuperSpeed and still have high-
> speed gadget drivers :-)
But this is the case right now. Or do you mean that you want to load (not
compile) the dummy_hcd with is_super_speed=1 and still be able to enumerate
HS gadgets?
> > following:
> > 1. if CONFIG_USB_GADGET_SUPERSPEED=true, existing gadget drivers are
> > still functional with dummy_hcd since as I already mentioned, they
> > will be enumerated through HS root hub and thus the gadget.speed will
> be set to HS.
> > This is true for all gadget drivers, including the once that don't
> > define SS descriptors.
>
> only due the module parameter, right ?
Due to the module parameter default value being FALSE, yes.
Best regards,
Tanya Brokhman
Consultant for Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
Hi,
On Wed, May 25, 2011 at 01:03:00PM +0300, Tanya Brokhman wrote:
> > > No, I'm afraid we're talking about different things here.
> > > As far as dummy_hcd is concerned:
> > > Dummy_hcd has a new module parameter that should be set to true if
> > the
> > > developer wishes the gadget driver to be connected to a SS root hub.
> > > Since at the moment most of the gadget drivers woun't work at SS
> > > connection (lack of SS descriptors) the default of this parameter is
> > > true and thus the connected gadget driver will be enumerated over a
> > HS
> > > root hub. Users don't have to remember t set anything in order to be
> > > able to work with dummy_hcd and the existing gadget drivers just as
> > they did up until now.
> > >
> > > We did add a new feature flag (CONFIG_USB_GADGET_SUPERSPEED) that is
> > > set during compilation. If that flag is defined, then (and only
> > then!)
> > > will the
> > > driver->speed be set to SS by composite_bind. Please keep in mind the
> >
> > and that's the only thing I'm asking you to remove :-)
>
> You want to remove the following change from in composite.c:
>
> @@ -1386,6 +1604,9 @@ int usb_composite_probe(struct usb_composite_driver
> *driver,
> driver->iProduct = driver->name;
> composite_driver.function = (char *) driver->name;
> composite_driver.driver.name = driver->name;
> +#ifdef CONFIG_USB_GADGET_SUPERSPEED
> + composite_driver.speed = USB_SPEED_SUPER; #endif /*
> +CONFIG_USB_GADGET_SUPERSPEED */
> composite = driver;
> composite_gadget_bind = bind;
>
> Right? I'm sorry, but I really don't understand why... :( Removing it is the
> same as not defining CONFIG_USB_GADGET_SUPERSPEED.
we still want to build a SuperSpeed dummy_hcd but have HS gadget drivers
:-)
> > we should be to compile dummy_hcd in SuperSpeed and still have high-
> > speed gadget drivers :-)
>
> But this is the case right now. Or do you mean that you want to load (not
> compile) the dummy_hcd with is_super_speed=1 and still be able to enumerate
> HS gadgets?
that too.
> > > following:
> > > 1. if CONFIG_USB_GADGET_SUPERSPEED=true, existing gadget drivers are
> > > still functional with dummy_hcd since as I already mentioned, they
> > > will be enumerated through HS root hub and thus the gadget.speed will
> > be set to HS.
> > > This is true for all gadget drivers, including the once that don't
> > > define SS descriptors.
> >
> > only due the module parameter, right ?
>
> Due to the module parameter default value being FALSE, yes.
Ok, so let's take your approach but change the speed in the gadget
driver structure, put your ifdef somewhere like here:
diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 1ba4bef..d02d6e8 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -1224,7 +1224,11 @@ composite_resume(struct usb_gadget *gadget)
/*-------------------------------------------------------------------------*/
static struct usb_gadget_driver composite_driver = {
+#ifdef CONFIG_USB_GADGET_SUPERSPEED
+ .speed = USB_SPEE_SUPER,
+#else
.speed = USB_SPEED_HIGH,
+#endif
.unbind = composite_unbind,
--
balbi
Hi Felipe,
> >
> > You want to remove the following change from in composite.c:
> >
> > @@ -1386,6 +1604,9 @@ int usb_composite_probe(struct
> > usb_composite_driver *driver,
> > driver->iProduct = driver->name;
> > composite_driver.function = (char *) driver->name;
> > composite_driver.driver.name = driver->name;
> > +#ifdef CONFIG_USB_GADGET_SUPERSPEED
> > + composite_driver.speed = USB_SPEED_SUPER; #endif /*
> > +CONFIG_USB_GADGET_SUPERSPEED */
> > composite = driver;
> > composite_gadget_bind = bind;
> >
> > Right? I'm sorry, but I really don't understand why... :( Removing it
> > is the same as not defining CONFIG_USB_GADGET_SUPERSPEED.
>
> we still want to build a SuperSpeed dummy_hcd but have HS gadget
> drivers
> :-)
But we do! I'm sorry, I'm getting a bit frustrated from not understanding
what bothers you here...
The above change in usb_composite_probe() doesn't prevent HS gadgets from
working with SS dummy_hcd.
Here is an example of the tests I ran on this patch series:
In my build CONFIG_USB_GADGET_SUPERSPEED=true so composite_driver.speed =
USBSPEED_SUPER.
1)
$> modprobe g_zero
G_zero was enumerated over HS root hub as a HS device. I ran our HS test
suite (from the UT framework) to test its functionality. It includes (HS)
descriptors verification, connect/disconnect and bulk in/out tests.
2)
$> modprobe dummy_hcd is_super_speed=1
$> modprobe g_zero
In this case the enumeration fails since g_zero didn't provide SS
descriptors!
I prepared another build, in which I used our generate_ss_descriptors() to
automaticly generate SS descriptors for g_zero gadget.
CONFIG_USB_GADGET_SUPERSPEED is still true:
3)
$> modprobe g_zero
G_zero was enumerated over HS root hub as a HS device although it has SS
descriptors. All HS UT that I described above passed
4)
$> modprobe dummy_hcd is_super_speed=1
$> modprobe g_zero
G_zero enumerated over SS root hub as a SS device. I verified that it's
functional with the SS test suite. It also verifies (SS) descriptors,
connect/disconnect and bulk in/out tests.
Please give me an example of what WOUN'T work with the above change.
>
> > > we should be to compile dummy_hcd in SuperSpeed and still have
> high-
> > > speed gadget drivers :-)
> >
> > But this is the case right now. Or do you mean that you want to load
> > (not
> > compile) the dummy_hcd with is_super_speed=1 and still be able to
> > enumerate HS gadgets?
>
> that too.
Well you can if at compile time you don't set CONFIG_USB_GADGET_SUPERSPEED
to true.
>
> > > > following:
> > > > 1. if CONFIG_USB_GADGET_SUPERSPEED=true, existing gadget drivers
> > > > are still functional with dummy_hcd since as I already mentioned,
> > > > they will be enumerated through HS root hub and thus the
> > > > gadget.speed will
> > > be set to HS.
> > > > This is true for all gadget drivers, including the once that
> don't
> > > > define SS descriptors.
> > >
> > > only due the module parameter, right ?
> >
> > Due to the module parameter default value being FALSE, yes.
>
> Ok, so let's take your approach but change the speed in the gadget
> driver structure, put your ifdef somewhere like here:
>
> diff --git a/drivers/usb/gadget/composite.c
> b/drivers/usb/gadget/composite.c index 1ba4bef..d02d6e8 100644
> --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -1224,7 +1224,11 @@ composite_resume(struct usb_gadget *gadget) /*-
> -----------------------------------------------------------------------
> -*/
>
> static struct usb_gadget_driver composite_driver = {
> +#ifdef CONFIG_USB_GADGET_SUPERSPEED
> + .speed = USB_SPEE_SUPER,
> +#else
> .speed = USB_SPEED_HIGH,
> +#endif
>
> .unbind = composite_unbind,
>
I have no problem updating static struct usb_gadget_driver composite_driver
as you suggested but it seems the same as updating it @
usb_composite_probe()...
Best regards,
Tanya Brokhman
Consultant for Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
Hi,
On Wed, May 25, 2011 at 01:52:15PM +0300, Tanya Brokhman wrote:
> > > > > following:
> > > > > 1. if CONFIG_USB_GADGET_SUPERSPEED=true, existing gadget drivers
> > > > > are still functional with dummy_hcd since as I already mentioned,
> > > > > they will be enumerated through HS root hub and thus the
> > > > > gadget.speed will
> > > > be set to HS.
> > > > > This is true for all gadget drivers, including the once that
> > don't
> > > > > define SS descriptors.
> > > >
> > > > only due the module parameter, right ?
> > >
> > > Due to the module parameter default value being FALSE, yes.
> >
> > Ok, so let's take your approach but change the speed in the gadget
> > driver structure, put your ifdef somewhere like here:
> >
> > diff --git a/drivers/usb/gadget/composite.c
> > b/drivers/usb/gadget/composite.c index 1ba4bef..d02d6e8 100644
> > --- a/drivers/usb/gadget/composite.c
> > +++ b/drivers/usb/gadget/composite.c
> > @@ -1224,7 +1224,11 @@ composite_resume(struct usb_gadget *gadget) /*-
> > -----------------------------------------------------------------------
> > -*/
> >
> > static struct usb_gadget_driver composite_driver = {
> > +#ifdef CONFIG_USB_GADGET_SUPERSPEED
> > + .speed = USB_SPEE_SUPER,
> > +#else
> > .speed = USB_SPEED_HIGH,
> > +#endif
> >
> > .unbind = composite_unbind,
> >
>
> I have no problem updating static struct usb_gadget_driver composite_driver
> as you suggested but it seems the same as updating it @
> usb_composite_probe()...
still, you will have two places poking at the same field. It's better to
keep it at once, update only that and we will take your approach.
--
balbi
Hi Felipe
> > > ---
> > > -*/
> > >
> > > static struct usb_gadget_driver composite_driver = {
> > > +#ifdef CONFIG_USB_GADGET_SUPERSPEED
> > > + .speed = USB_SPEE_SUPER,
> > > +#else
> > > .speed = USB_SPEED_HIGH,
> > > +#endif
> > >
> > > .unbind = composite_unbind,
> > >
> >
> > I have no problem updating static struct usb_gadget_driver
> > composite_driver as you suggested but it seems the same as updating
> it
> > @ usb_composite_probe()...
>
> still, you will have two places poking at the same field. It's better
> to keep it at once, update only that and we will take your approach.
>
Ok so just to make sure I understand you correctly:
You want me to remove the modification made to usb_composite_probe() and
instead add:
static struct usb_gadget_driver composite_driver = {
+#ifdef CONFIG_USB_GADGET_SUPERSPEED
+ .speed = USB_SPEE_SUPER,
+#else
.speed = USB_SPEED_HIGH,
+#endif
.unbind = composite_unbind,
And then you'll be ok with change? Or is there anything else?
If this is it then I'm relieved :) and of course will update the code.
Best regards,
Tanya Brokhman
Consultant for Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
Hi,
On Wed, May 25, 2011 at 02:33:18PM +0300, Tanya Brokhman wrote:
> Ok so just to make sure I understand you correctly:
> You want me to remove the modification made to usb_composite_probe() and
> instead add:
> static struct usb_gadget_driver composite_driver = {
> +#ifdef CONFIG_USB_GADGET_SUPERSPEED
> + .speed = USB_SPEE_SUPER,
> +#else
> .speed = USB_SPEED_HIGH,
> +#endif
> .unbind = composite_unbind,
>
> And then you'll be ok with change? Or is there anything else?
>
> If this is it then I'm relieved :) and of course will update the code.
yeah, that's all. Just be sure not to introduce the typo I introduced
(missed D in SPEED) :-)
Then I'll hold on to those patches for 2.6.41 merge window.
--
balbi
On Wed, 25 May 2011, Tanya Brokhman wrote:
> Ok so just to make sure I understand you correctly:
> You want me to remove the modification made to usb_composite_probe() and
> instead add:
> static struct usb_gadget_driver composite_driver = {
> +#ifdef CONFIG_USB_GADGET_SUPERSPEED
> + .speed = USB_SPEE_SUPER,
> +#else
> .speed = USB_SPEED_HIGH,
> +#endif
> .unbind = composite_unbind,
>
> And then you'll be ok with change? Or is there anything else?
>
> If this is it then I'm relieved :) and of course will update the code.
I have looked this over more carefully. It turns out that both of you
have misunderstood the purpose of CONFIG_USB_GADGET_DUALSPEED (and by
extension, CONFIG_USB_GADGET_SUPERSPEED). In fact, the existing
Kconfig file is also wrong.
The _only_ reason for CONFIG_USB_GADGET_DUALSPEED is so that gadget
drivers can use conditional compilation to avoid including the
high-speed descriptors when the UDC doesn't support high-speed
operation. That's all. This means that the
CONFIG_USB_GAGDET_DUALSPEED option does not need to be
user-controllable in Kconfig. It should default to N, and UDC drivers
that support high speed should select it.
The same should be true of CONFIG_USB_GADGET_HIGHSPEED. It should not
be user-controllable. It should default to N, and UDC drivers that
support SuperSpeed operation (after these patches, only dummy-hcd)
should select it.
There remains the other question, about whether composite_driver.speed
should be set to USB_SPEED_SUPER. I think the matter can be settled at
runtime. Iterate through all the function drivers; if all of them
support SuperSpeed and CONFIG_USB_GADGET_SUPERSPEED is enabled then set
composite_driver.speed to USB_SPEED_SUPER. Otherwise, if all of them
support high speed and CONFIG_USB_GADGET_DUALSPEED is enabled then set
composite_driver.speed to USB_SPEED_HIGH. Otherwise set it to
USB_SPEED_FULL.
Alan Stern
On Wed, 25 May 2011, Alan Stern wrote:
> I have looked this over more carefully. It turns out that both of you
> have misunderstood the purpose of CONFIG_USB_GADGET_DUALSPEED (and by
> extension, CONFIG_USB_GADGET_SUPERSPEED). In fact, the existing
> Kconfig file is also wrong.
>
> The _only_ reason for CONFIG_USB_GADGET_DUALSPEED is so that gadget
> drivers can use conditional compilation to avoid including the
> high-speed descriptors when the UDC doesn't support high-speed
> operation. That's all. This means that the
> CONFIG_USB_GAGDET_DUALSPEED option does not need to be
> user-controllable in Kconfig. It should default to N, and UDC drivers
> that support high speed should select it.
In other words, we should merge the following patch and the new
SuperSpeed support should follow the same pattern.
Alan Stern
---------------------------------------------------------------------
This patch (as1468) changes the Kconfig definition for
USB_GADGET_DUALSPEED. This option is determined entirely by which
device controller drivers are to be built, through Select statements;
it does not need to be (and should not be) configurable by the user.
Also, the "default n" line is superfluous -- everything defaults to N.
Signed-off-by: Alan Stern <[email protected]>
---
drivers/usb/gadget/Kconfig | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
Index: usb-2.6/drivers/usb/gadget/Kconfig
===================================================================
--- usb-2.6.orig/drivers/usb/gadget/Kconfig
+++ usb-2.6/drivers/usb/gadget/Kconfig
@@ -597,13 +597,10 @@ config USB_DUMMY_HCD
endchoice
+# Selected by UDC drivers that support high-speed operation.
config USB_GADGET_DUALSPEED
bool
depends on USB_GADGET
- default n
- help
- Means that gadget drivers should include extra descriptors
- and code to handle dual-speed controllers.
#
# USB Gadget Drivers
Hi Alan,
> I have looked this over more carefully. It turns out that both of you
> have misunderstood the purpose of CONFIG_USB_GADGET_DUALSPEED (and by
> extension, CONFIG_USB_GADGET_SUPERSPEED). In fact, the existing
> Kconfig file is also wrong.
>
> The _only_ reason for CONFIG_USB_GADGET_DUALSPEED is so that gadget
> drivers can use conditional compilation to avoid including the
> high-speed descriptors when the UDC doesn't support high-speed
> operation. That's all. This means that the
> CONFIG_USB_GAGDET_DUALSPEED option does not need to be
> user-controllable in Kconfig. It should default to N, and UDC drivers
> that support high speed should select it.
>
> The same should be true of CONFIG_USB_GADGET_HIGHSPEED. It should not
> be user-controllable. It should default to N, and UDC drivers that
> support SuperSpeed operation (after these patches, only dummy-hcd)
> should select it.
Ok, agreed. Thanks for the clarification. I saw your patch, I'll update
mine in the same way.
> There remains the other question, about whether composite_driver.speed
> should be set to USB_SPEED_SUPER. I think the matter can be settled at
> runtime. Iterate through all the function drivers; if all of them
> support SuperSpeed and CONFIG_USB_GADGET_SUPERSPEED is enabled then set
> composite_driver.speed to USB_SPEED_SUPER.
Not sure how to verify this. I need to know whether the driver that is
registered with the UDC is SS or not. This is before the function drivers
are binded to it. So how can I verify at that point that the function
drivers that will bind to this driver will provide SS descriptors?
(I'm sorry, I don't have the ability to view the code at the moment and
due to the time differences between us I don't want to leave this question
for tomorrow and loose another day...)
>Otherwise, if all of them
> support high speed and CONFIG_USB_GADGET_DUALSPEED is enabled then set
> composite_driver.speed to USB_SPEED_HIGH. Otherwise set it to
> USB_SPEED_FULL.
>
> Alan Stern
>
Thanks,
Tanya Brokhman
--
Sent by an consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
On Wed, 25 May 2011, Brokhman Tatyana wrote:
> Hi Alan,
>
> > I have looked this over more carefully. It turns out that both of you
> > have misunderstood the purpose of CONFIG_USB_GADGET_DUALSPEED (and by
> > extension, CONFIG_USB_GADGET_SUPERSPEED). In fact, the existing
> > Kconfig file is also wrong.
> >
> > The _only_ reason for CONFIG_USB_GADGET_DUALSPEED is so that gadget
> > drivers can use conditional compilation to avoid including the
> > high-speed descriptors when the UDC doesn't support high-speed
> > operation. That's all. This means that the
> > CONFIG_USB_GAGDET_DUALSPEED option does not need to be
> > user-controllable in Kconfig. It should default to N, and UDC drivers
> > that support high speed should select it.
> >
> > The same should be true of CONFIG_USB_GADGET_HIGHSPEED. It should not
> > be user-controllable. It should default to N, and UDC drivers that
> > support SuperSpeed operation (after these patches, only dummy-hcd)
> > should select it.
>
> Ok, agreed. Thanks for the clarification. I saw your patch, I'll update
> mine in the same way.
Good. If Felipe doesn't object to my patch, I'll send it to Greg.
> > There remains the other question, about whether composite_driver.speed
> > should be set to USB_SPEED_SUPER. I think the matter can be settled at
> > runtime. Iterate through all the function drivers; if all of them
> > support SuperSpeed and CONFIG_USB_GADGET_SUPERSPEED is enabled then set
> > composite_driver.speed to USB_SPEED_SUPER.
>
> Not sure how to verify this. I need to know whether the driver that is
> registered with the UDC is SS or not. This is before the function drivers
> are binded to it. So how can I verify at that point that the function
> drivers that will bind to this driver will provide SS descriptors?
> (I'm sorry, I don't have the ability to view the code at the moment and
> due to the time differences between us I don't want to leave this question
> for tomorrow and loose another day...)
I'm not sure about this either. I have never used the composite
framework so I'm not familiar with its details. This has to be decided
before the composite gadget driver registers with the UDC driver ...
and surely that can't happen until all the function drivers have
registered with the composite driver? After all, the gadget can't be
enumerated until all the function drivers are registered.
Maybe this logic can be put in usb_add_function()? That routine
already deals with issues involving device speed and available
descriptors.
Alan Stern
composite_driver.speed to USB_SPEED_SUPER.
>>
>> Not sure how to verify this. I need to know whether the driver that is
>> registered with the UDC is SS or not. This is before the function
>> drivers
>> are binded to it. So how can I verify at that point that the function
>> drivers that will bind to this driver will provide SS descriptors?
>> (I'm sorry, I don't have the ability to view the code at the moment and
>> due to the time differences between us I don't want to leave this
>> question
>> for tomorrow and loose another day...)
>
> I'm not sure about this either. I have never used the composite
> framework so I'm not familiar with its details. This has to be decided
> before the composite gadget driver registers with the UDC driver ...
Right, but at this point there is no way of knowing what function drivers
will bind to it and what descriptors they will provide. Most of the
function drivers allocate their descriptors at bind() that occurs after
the UDC already registers.
> and surely that can't happen until all the function drivers have
> registered with the composite driver? After all, the gadget can't be
> enumerated until all the function drivers are registered.
Right, but it doesn't mean that the composite driver can't register with
the UDC until all the function drivers are registered. Actually, the
composite driver binds when no function driver has yet registered with it.
> Maybe this logic can be put in usb_add_function()? That routine
> already deals with issues involving device speed and available
> descriptors.
That's too late. The composite driver is registered by the time
usb_add_function() is called.
Tanya Brokhman
--
Sent by an consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
On Wed, 25 May 2011, Brokhman Tatyana wrote:
>
> composite_driver.speed to USB_SPEED_SUPER.
> >>
> >> Not sure how to verify this. I need to know whether the driver that is
> >> registered with the UDC is SS or not. This is before the function
> >> drivers
> >> are binded to it. So how can I verify at that point that the function
> >> drivers that will bind to this driver will provide SS descriptors?
> >> (I'm sorry, I don't have the ability to view the code at the moment and
> >> due to the time differences between us I don't want to leave this
> >> question
> >> for tomorrow and loose another day...)
> >
> > I'm not sure about this either. I have never used the composite
> > framework so I'm not familiar with its details. This has to be decided
> > before the composite gadget driver registers with the UDC driver ...
> Right, but at this point there is no way of knowing what function drivers
> will bind to it and what descriptors they will provide. Most of the
> function drivers allocate their descriptors at bind() that occurs after
> the UDC already registers.
Well, there must be an appropriate spot to do this.
It looks like you need to add a "max_speed" field to struct
usb_composite_driver. Each driver will initialize this to the highest
speed it supports, and it must guarantee that the necessary descriptors
are available.
Since these structures are passed to usb_composite_probe() before the
gadget driver is registered, the necessary calculations can be done
there.
Alan Stern
Hi Alan, Felipe,
> > composite_driver.speed to USB_SPEED_SUPER.
> > >>
> > >> Not sure how to verify this. I need to know whether the driver
> that is
> > >> registered with the UDC is SS or not. This is before the function
> > >> drivers
> > >> are binded to it. So how can I verify at that point that the
> function
> > >> drivers that will bind to this driver will provide SS descriptors?
> > >> (I'm sorry, I don't have the ability to view the code at the
> moment and
> > >> due to the time differences between us I don't want to leave this
> > >> question
> > >> for tomorrow and loose another day...)
> > >
> > > I'm not sure about this either. I have never used the composite
> > > framework so I'm not familiar with its details. This has to be
> decided
> > > before the composite gadget driver registers with the UDC driver
> ...
> > Right, but at this point there is no way of knowing what function
> drivers
> > will bind to it and what descriptors they will provide. Most of the
> > function drivers allocate their descriptors at bind() that occurs
> after
> > the UDC already registers.
>
> Well, there must be an appropriate spot to do this.
Unfortunately in the current implementation there isn't. We must set the
driver speed at usb_composite_probe().
The (not full) call stack for composite driver registration is:
1. usb_composite_probe()
2. usb_gadget_probe_driver()
3. composite_bind() - here the device descriptor is updated (fields such as
bMaxPacketSize0, idVendor, etc)
4. the gadgets "bind" function is called. For example for g_zero it's
zero_bind(). Only at this point will the function driver register it's
configurations and for each registered config - add the appropriate
functions.
So as you can see, whether the gadget driver supports/or not SS can be
determined only at the end of the registration sequence, whether as far as
UDC is concerned, we need to set the correct driver speed at the very
beginning - at usb_composite_probe().
Felipe - please correct me if I'm mistaken. Is there a way to find out at
usb_composite_probe() if the registered function driver will support SS?
> It looks like you need to add a "max_speed" field to struct
> usb_composite_driver. Each driver will initialize this to the highest
> speed it supports, and it must guarantee that the necessary descriptors
> are available.
I also thought of that. This can be done...
Just to make sure we're on the same page:
We can add a "max_sup_speed" field to struct usb_composite_driver. Each of
the Gadget drivers will set this field prior to calling
usb_composite_probe(). Then, in usb_composite_probe(), we'll update the
composite_driver.speed as follows:
composite_driver.speed = min(composite_driver.speed,
driver->max_sup_speed);
Of course for SS we'll update the composite_driver.speed @ static struct
usb_gadget_driver composite_driver, as we agreed with Felipe.
How does that sound? Felipe - it seems to me that these should cover your
hesitations about updating the driver speed :)
Now that I think about it, the above will be true for HS as well. I mean the
current speed of composite_driver is set always to HS but if there is a
function driver that supports only FS then the above will update
composite_driver.speed to FS.
Best regards,
Tanya Brokhman
Consultant for Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
On Thu, 26 May 2011, Tanya Brokhman wrote:
> > > Right, but at this point there is no way of knowing what function
> > drivers
> > > will bind to it and what descriptors they will provide. Most of the
> > > function drivers allocate their descriptors at bind() that occurs
> > after
> > > the UDC already registers.
> >
> > Well, there must be an appropriate spot to do this.
>
> Unfortunately in the current implementation there isn't.
Sure there is. You describe a good possibility later on in your
email...
> > It looks like you need to add a "max_speed" field to struct
> > usb_composite_driver. Each driver will initialize this to the highest
> > speed it supports, and it must guarantee that the necessary descriptors
> > are available.
>
> I also thought of that. This can be done...
> Just to make sure we're on the same page:
> We can add a "max_sup_speed" field to struct usb_composite_driver. Each of
> the Gadget drivers will set this field prior to calling
> usb_composite_probe(). Then, in usb_composite_probe(), we'll update the
> composite_driver.speed as follows:
>
> composite_driver.speed = min(composite_driver.speed,
> driver->max_sup_speed);
Just call it max_speed. You can mention in the kerneldoc that it is
the maximum speed supported by the driver.
> Of course for SS we'll update the composite_driver.speed @ static struct
> usb_gadget_driver composite_driver, as we agreed with Felipe.
>
> How does that sound? Felipe - it seems to me that these should cover your
> hesitations about updating the driver speed :)
>
> Now that I think about it, the above will be true for HS as well. I mean the
> current speed of composite_driver is set always to HS but if there is a
> function driver that supports only FS then the above will update
> composite_driver.speed to FS.
That's right. The same solution will work for both cases.
Alan Stern
Hi Alan,
>> I also thought of that. This can be done...
>> Just to make sure we're on the same page:
>> We can add a "max_sup_speed" field to struct usb_composite_driver. Each
>> of
>> the Gadget drivers will set this field prior to calling
>> usb_composite_probe(). Then, in usb_composite_probe(), we'll update the
>> composite_driver.speed as follows:
>>
>> composite_driver.speed = min(composite_driver.speed,
>> driver->max_sup_speed);
>
> Just call it max_speed. You can mention in the kerneldoc that it is
> the maximum speed supported by the driver.
No problem. I understand that you agree to this approach?
I was going to wait for Felipe to comment but he'll be out of reach for
several days so I'll post a new patch set on Sunday.
Thanks for you help!
Tanya Brokhman
--
Sent by an consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Hi Alan,
> This patch (as1468) changes the Kconfig definition for
> USB_GADGET_DUALSPEED. This option is determined entirely by which
> device controller drivers are to be built, through Select statements;
> it does not need to be (and should not be) configurable by the user.
>
> Also, the "default n" line is superfluous -- everything defaults to N.
>
> Signed-off-by: Alan Stern <[email protected]>
>
> ---
>
> drivers/usb/gadget/Kconfig | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> Index: usb-2.6/drivers/usb/gadget/Kconfig
> ===================================================================
> --- usb-2.6.orig/drivers/usb/gadget/Kconfig
> +++ usb-2.6/drivers/usb/gadget/Kconfig
> @@ -597,13 +597,10 @@ config USB_DUMMY_HCD
>
> endchoice
>
> +# Selected by UDC drivers that support high-speed operation.
> config USB_GADGET_DUALSPEED
> bool
> depends on USB_GADGET
> - default n
> - help
> - Means that gadget drivers should include extra descriptors
> - and code to handle dual-speed controllers.
>
> #
> # USB Gadget Drivers
>
I was making the same change for SS and I think that if you're about to send
this patch to Greg, it might need a small addition:
At the moment dummy_hcd supports high_speed so I think if this option is no
longer configurable by user you should set this flag when dummy_hcd is
selected:
config USB_DUMMY_HCD
tristate
depends on USB_GADGET_DUMMY_HCD
default USB_GADGET
select USB_GADGET_SELECTED
select USB_GADGET_DUALSPEED
Best regards,
Tanya Brokhman
Consultant for Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
On Sat, 28 May 2011, Tanya Brokhman wrote:
> I was making the same change for SS and I think that if you're about to send
> this patch to Greg, it might need a small addition:
> At the moment dummy_hcd supports high_speed so I think if this option is no
> longer configurable by user you should set this flag when dummy_hcd is
> selected:
>
> config USB_DUMMY_HCD
> tristate
> depends on USB_GADGET_DUMMY_HCD
> default USB_GADGET
> select USB_GADGET_SELECTED
> select USB_GADGET_DUALSPEED
Right now USB_GADGET_DUALSPEED is selected under USB_GADGET_DUMMY_HCD,
which is the right place for it, I think. Same as all the other UDC
drivers.
Alan Stern
Hi,
On Wed, May 25, 2011 at 10:39:24AM -0400, Alan Stern wrote:
> This patch (as1468) changes the Kconfig definition for
> USB_GADGET_DUALSPEED. This option is determined entirely by which
> device controller drivers are to be built, through Select statements;
> it does not need to be (and should not be) configurable by the user.
>
> Also, the "default n" line is superfluous -- everything defaults to N.
>
> Signed-off-by: Alan Stern <[email protected]>
this patch makes a lot of sense. FWIW:
Acked-by: Felipe Balbi <[email protected]>
> ---
>
> drivers/usb/gadget/Kconfig | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> Index: usb-2.6/drivers/usb/gadget/Kconfig
> ===================================================================
> --- usb-2.6.orig/drivers/usb/gadget/Kconfig
> +++ usb-2.6/drivers/usb/gadget/Kconfig
> @@ -597,13 +597,10 @@ config USB_DUMMY_HCD
>
> endchoice
>
> +# Selected by UDC drivers that support high-speed operation.
> config USB_GADGET_DUALSPEED
> bool
> depends on USB_GADGET
> - default n
> - help
> - Means that gadget drivers should include extra descriptors
> - and code to handle dual-speed controllers.
>
> #
> # USB Gadget Drivers
>
--
balbi