[PATCH 0/3] usb:gadget: Add SuperSpeed Function Power Management

[PATCH 0/3] usb:gadget: Add SuperSpeed Function Power Management

[Amit Blay]

This patch series adds Function Power Management to the Gadget Framework for
SuperSpeed USB.

To test the new code, g_zero gadget was updated to support SuperSpeed, and
f_sourcesink function was updated to support SuperSpeed specific handlers.

We used dummy_hcd to emulate UDC & HCD that support SuperSpeed.
We tested the development with our unittest environment which is based on
libusb & GoogleTest. This environment is submitted seperately by
tlinder@codeaurora.org.


Amit Blay (3):
  usb: Add SuperSpeed support to g_zero gadget
  usb:gadget: Add SuperSpeed function power management
  usb:gadget: SuperSpeed function power management testing support

 drivers/usb/gadget/dummy_hcd.c    |    3 +-
 drivers/usb/gadget/f_loopback.c   |   57 ++++++++++-
 drivers/usb/gadget/f_sourcesink.c |  217 ++++++++++++++++++++++++++++++++++++-
 drivers/usb/gadget/udc-core.c     |    2 +-
 drivers/usb/gadget/zero.c         |    8 +-
 include/linux/usb/ch9.h           |    8 ++
 include/linux/usb/gadget.h        |   35 +++++--
 7 files changed, 315 insertions(+), 15 deletions(-)

-- 
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.

[RFC/PATCH 1/3] usb: Add SuperSpeed support to g_zero gadget

[Amit Blay]

This patch adds SuperSpeed descriptors to the g_zero gadget.
The SuperSpeed descriptors were added both for f_soursesink and f_loopback
configurations of the g_zero gadget.

Signed-off-by: Tatyana Brokhman <tlinder-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
Signed-off-by: Amit Blay <ablay-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>

---
 drivers/usb/gadget/f_loopback.c   |   55 ++++++++++++++++++++++++++++++++++++-
 drivers/usb/gadget/f_sourcesink.c |   55 ++++++++++++++++++++++++++++++++++++-
 drivers/usb/gadget/zero.c         |    2 +-
 3 files changed, 109 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/f_loopback.c b/drivers/usb/gadget/f_loopback.c
index 3756326..ca660d4 100644
--- a/drivers/usb/gadget/f_loopback.c
+++ b/drivers/usb/gadget/f_loopback.c
@@ -118,6 +118,49 @@ static struct usb_descriptor_header *hs_loopback_descs[] = {
 	NULL,
 };
 
+/* super speed support: */
+
+static struct usb_endpoint_descriptor ss_loop_source_desc = {
+	.bLength =		USB_DT_ENDPOINT_SIZE,
+	.bDescriptorType =	USB_DT_ENDPOINT,
+
+	.bmAttributes =		USB_ENDPOINT_XFER_BULK,
+	.wMaxPacketSize =	cpu_to_le16(1024),
+};
+
+struct usb_ss_ep_comp_descriptor ss_loop_source_comp_desc = {
+	.bLength =		USB_DT_SS_EP_COMP_SIZE,
+	.bDescriptorType =	USB_DT_SS_ENDPOINT_COMP,
+	.bMaxBurst =		0,
+	.bmAttributes =		0,
+	.wBytesPerInterval =	0,
+};
+
+static struct usb_endpoint_descriptor ss_loop_sink_desc = {
+	.bLength =		USB_DT_ENDPOINT_SIZE,
+	.bDescriptorType =	USB_DT_ENDPOINT,
+
+	.bmAttributes =		USB_ENDPOINT_XFER_BULK,
+	.wMaxPacketSize =	cpu_to_le16(1024),
+};
+
+struct usb_ss_ep_comp_descriptor ss_loop_sink_comp_desc = {
+	.bLength =		USB_DT_SS_EP_COMP_SIZE,
+	.bDescriptorType =	USB_DT_SS_ENDPOINT_COMP,
+	.bMaxBurst =		0,
+	.bmAttributes =		0,
+	.wBytesPerInterval =	0,
+};
+
+static struct usb_descriptor_header *ss_loopback_descs[] = {
+	(struct usb_descriptor_header *) &loopback_intf,
+	(struct usb_descriptor_header *) &ss_loop_source_desc,
+	(struct usb_descriptor_header *) &ss_loop_source_comp_desc,
+	(struct usb_descriptor_header *) &ss_loop_sink_desc,
+	(struct usb_descriptor_header *) &ss_loop_sink_comp_desc,
+	NULL,
+};
+
 /* function-specific strings: */
 
 static struct usb_string strings_loopback[] = {
@@ -175,8 +218,18 @@ autoconf_fail:
 		f->hs_descriptors = hs_loopback_descs;
 	}
 
+	/* support super speed hardware */
+	if (gadget_is_superspeed(c->cdev->gadget)) {
+		ss_loop_source_desc.bEndpointAddress =
+				fs_loop_source_desc.bEndpointAddress;
+		ss_loop_sink_desc.bEndpointAddress =
+				fs_loop_sink_desc.bEndpointAddress;
+		f->ss_descriptors = ss_loopback_descs;
+	}
+
 	DBG(cdev, "%s speed %s: IN/%s, OUT/%s\n",
-			gadget_is_dualspeed(c->cdev->gadget) ? "dual" : "full",
+	    (gadget_is_superspeed(c->cdev->gadget) ? "super" :
+	     (gadget_is_dualspeed(c->cdev->gadget) ? "dual" : "full")),
 			f->name, loop->in_ep->name, loop->out_ep->name);
 	return 0;
 }
diff --git a/drivers/usb/gadget/f_sourcesink.c b/drivers/usb/gadget/f_sourcesink.c
index caf2f95..5247f07 100644
--- a/drivers/usb/gadget/f_sourcesink.c
+++ b/drivers/usb/gadget/f_sourcesink.c
@@ -131,6 +131,49 @@ static struct usb_descriptor_header *hs_source_sink_descs[] = {
 	NULL,
 };
 
+/* super speed support: */
+
+static struct usb_endpoint_descriptor ss_source_desc = {
+	.bLength =		USB_DT_ENDPOINT_SIZE,
+	.bDescriptorType =	USB_DT_ENDPOINT,
+
+	.bmAttributes =		USB_ENDPOINT_XFER_BULK,
+	.wMaxPacketSize =	cpu_to_le16(1024),
+};
+
+struct usb_ss_ep_comp_descriptor ss_source_comp_desc = {
+	.bLength =		USB_DT_SS_EP_COMP_SIZE,
+	.bDescriptorType =	USB_DT_SS_ENDPOINT_COMP,
+	.bMaxBurst =		0,
+	.bmAttributes =		0,
+	.wBytesPerInterval =	0,
+};
+
+static struct usb_endpoint_descriptor ss_sink_desc = {
+	.bLength =		USB_DT_ENDPOINT_SIZE,
+	.bDescriptorType =	USB_DT_ENDPOINT,
+
+	.bmAttributes =		USB_ENDPOINT_XFER_BULK,
+	.wMaxPacketSize =	cpu_to_le16(1024),
+};
+
+struct usb_ss_ep_comp_descriptor ss_sink_comp_desc = {
+	.bLength =		USB_DT_SS_EP_COMP_SIZE,
+	.bDescriptorType =	USB_DT_SS_ENDPOINT_COMP,
+	.bMaxBurst =		0,
+	.bmAttributes =		0,
+	.wBytesPerInterval =	0,
+};
+
+static struct usb_descriptor_header *ss_source_sink_descs[] = {
+	(struct usb_descriptor_header *) &source_sink_intf,
+	(struct usb_descriptor_header *) &ss_source_desc,
+	(struct usb_descriptor_header *) &ss_source_comp_desc,
+	(struct usb_descriptor_header *) &ss_sink_desc,
+	(struct usb_descriptor_header *) &ss_sink_comp_desc,
+	NULL,
+};
+
 /* function-specific strings: */
 
 static struct usb_string strings_sourcesink[] = {
@@ -187,8 +230,18 @@ autoconf_fail:
 		f->hs_descriptors = hs_source_sink_descs;
 	}
 
+	/* support super speed hardware */
+	if (gadget_is_superspeed(c->cdev->gadget)) {
+		ss_source_desc.bEndpointAddress =
+				fs_source_desc.bEndpointAddress;
+		ss_sink_desc.bEndpointAddress =
+				fs_sink_desc.bEndpointAddress;
+		f->ss_descriptors = ss_source_sink_descs;
+	}
+
 	DBG(cdev, "%s speed %s: IN/%s, OUT/%s\n",
-			gadget_is_dualspeed(c->cdev->gadget) ? "dual" : "full",
+	    (gadget_is_superspeed(c->cdev->gadget) ? "super" :
+	     (gadget_is_dualspeed(c->cdev->gadget) ? "dual" : "full")),
 			f->name, ss->in_ep->name, ss->out_ep->name);
 	return 0;
 }
diff --git a/drivers/usb/gadget/zero.c b/drivers/usb/gadget/zero.c
index af7e7c3..00e2fd2 100644
--- a/drivers/usb/gadget/zero.c
+++ b/drivers/usb/gadget/zero.c
@@ -340,7 +340,7 @@ static struct usb_composite_driver zero_driver = {
 	.name		= "zero",
 	.dev		= &device_desc,
 	.strings	= dev_strings,
-	.max_speed	= USB_SPEED_HIGH,
+	.max_speed	= USB_SPEED_SUPER,
 	.unbind		= zero_unbind,
 	.suspend	= zero_suspend,
 	.resume		= zero_resume,
-- 
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.


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC/PATCH 2/3] usb:gadget: Add SuperSpeed function power management

[Amit Blay]

1. Add missing definitions in ch9.h for requests tarageted to an
Interface: GET_STATUS & SET_FEATURE(FUNC_SUSPEND).

2. Add func_wakeup api to usb_gadget_ops.
Super-Speed device must provide interface number to the UDC when
it triggers remote wakeup (function wake).
The func_wakeup api is used instead of the wakeup api, when the
gadget is connected in Super-Speed. The wakeup api is left as is,
and it is used when the gadget is connected in High-Speed. Therefore,
no change is required in non Super-Speed UDCs.

Signed-off-by: Amit Blay <ablay@codeaurora.org>

---
 drivers/usb/gadget/udc-core.c |    2 +-
 drivers/usb/gadget/zero.c     |    6 +++++-
 include/linux/usb/ch9.h       |    8 ++++++++
 include/linux/usb/gadget.h    |   35 +++++++++++++++++++++++++++--------
 4 files changed, 41 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/gadget/udc-core.c b/drivers/usb/gadget/udc-core.c
index 05ba472..beb7e89 100644
--- a/drivers/usb/gadget/udc-core.c
+++ b/drivers/usb/gadget/udc-core.c
@@ -347,7 +347,7 @@ static ssize_t usb_udc_srp_store(struct device *dev,
 	struct usb_udc		*udc = dev_get_drvdata(dev);
 
 	if (sysfs_streq(buf, "1"))
-		usb_gadget_wakeup(udc->gadget);
+		usb_gadget_wakeup(udc->gadget, 0);
 
 	return n;
 }
diff --git a/drivers/usb/gadget/zero.c b/drivers/usb/gadget/zero.c
index 00e2fd2..a5d13c6 100644
--- a/drivers/usb/gadget/zero.c
+++ b/drivers/usb/gadget/zero.c
@@ -239,7 +239,11 @@ static void zero_autoresume(unsigned long _c)
 	 * because of some direct user request.
 	 */
 	if (g->speed != USB_SPEED_UNKNOWN) {
-		int status = usb_gadget_wakeup(g);
+		/*
+		 * The single interface of the current configuration
+		 * triggers the wakeup.
+		 */
+		int status = usb_gadget_wakeup(g, 1);
 		INFO(cdev, "%s --> %d\n", __func__, status);
 	}
 }
diff --git a/include/linux/usb/ch9.h b/include/linux/usb/ch9.h
index 0fd3fbd..404c10e 100644
--- a/include/linux/usb/ch9.h
+++ b/include/linux/usb/ch9.h
@@ -142,7 +142,13 @@
 #define USB_DEVICE_LTM_ENABLE	50	/* dev may send LTM */
 #define USB_INTRF_FUNC_SUSPEND	0	/* function suspend */
 
+/*
+ * Function Suspend Options as defined by USB 3.0
+ * See USB 3.0 spec Table 9-7
+ */
 #define USB_INTR_FUNC_SUSPEND_OPT_MASK	0xFF00
+#define USB_INTR_FUNC_SUSPEND_SUSP		1 /* function suspend option */
+#define USB_INTR_FUNC_SUSPEND_RWAKE_EN	2 /* function wake enabled option */
 
 #define USB_ENDPOINT_HALT		0	/* IN/OUT will STALL */
 
@@ -150,6 +156,8 @@
 #define USB_DEV_STAT_U1_ENABLED		2	/* transition into U1 state */
 #define USB_DEV_STAT_U2_ENABLED		3	/* transition into U2 state */
 #define USB_DEV_STAT_LTM_ENABLED	4	/* Latency tolerance messages */
+#define USB_INTR_STAT_RWAKE_CAP		0	/* Function wake capable */
+#define USB_INTR_STAT_RWAKE_EN			1	/* Function wake enabled */
 
 /**
  * struct usb_ctrlrequest - SETUP data for a USB device control request
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index 087f4b9..3ebbc11 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -458,7 +458,11 @@ struct usb_gadget_ops {
 	int	(*pullup) (struct usb_gadget *, int is_on);
 	int	(*ioctl)(struct usb_gadget *,
 				unsigned code, unsigned long param);
+
+	/* USB 3.0 additions */
 	void	(*get_config_params)(struct usb_dcd_config_params *);
+	int     (*func_wakeup)(struct usb_gadget *, int interface_id);
+
 	int	(*udc_start)(struct usb_gadget *,
 			struct usb_gadget_driver *);
 	int	(*udc_stop)(struct usb_gadget *,
@@ -607,21 +611,36 @@ static inline int usb_gadget_frame_number(struct usb_gadget *gadget)
 /**
  * usb_gadget_wakeup - tries to wake up the host connected to this gadget
  * @gadget: controller used to wake up the host
+ * @interface_id: id of the first interface of the function that
+ *	triggered the wake up
  *
  * Returns zero on success, else negative error code if the hardware
  * doesn't support such attempts, or its support has not been enabled
  * by the usb host.  Drivers must return device descriptors that report
  * their ability to support this, or hosts won't enable it.
+ * For Super-Speed devices only, the gadget should provide the
+ * id of the first interface that triggered the wake up
+ * (function wake). For non Super-Speed devices, the value of
+ * this parameter doesn't matter.
  *
- * This may also try to use SRP to wake the host and start enumeration,
- * even if OTG isn't otherwise in use.  OTG devices may also start
- * remote wakeup even when hosts don't explicitly enable it.
+ * This may also try to use SRP to wake the host and start
+ * enumeration, even if OTG isn't otherwise in use.  OTG devices
+ * may also start remote wakeup even when hosts don't explicitly
+ * enable it.
  */
-static inline int usb_gadget_wakeup(struct usb_gadget *gadget)
+static inline int usb_gadget_wakeup(struct usb_gadget *gadget, int interface_id)
 {
-	if (!gadget->ops->wakeup)
-		return -EOPNOTSUPP;
-	return gadget->ops->wakeup(gadget);
+	if (gadget->speed == USB_SPEED_SUPER) {
+		if (!gadget->ops->func_wakeup)
+			return -EOPNOTSUPP;
+
+		return gadget->ops->func_wakeup(gadget, interface_id);
+	} else {
+		if (!gadget->ops->wakeup)
+			return -EOPNOTSUPP;
+
+		return gadget->ops->wakeup(gadget);
+	}
 }
 
 /**
-- 
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.

[RFC/PATCH 3/3] usb:gadget: SuperSpeed function power management testing support

[Amit Blay]

1. Fix dummy_hcd to let the function handle GET_STATUS(Interface) request.
2. Fix a bug in f_loopback Gadget which updated the bmAttribute of
f_sourcesink instead of f_loopback.
3. Update f_sourcesink Gadget to support function suspend & wakeup.
This is done by adding get_status() & func_suspend() handlers.
The current status of the function is controlable via debug FS:
(wakeup capable, wakeup enabled, suspended).

Signed-off-by: Amit Blay <ablay@codeaurora.org>

---
 drivers/usb/gadget/dummy_hcd.c    |    3 +-
 drivers/usb/gadget/f_loopback.c   |    2 +-
 drivers/usb/gadget/f_sourcesink.c |  162 +++++++++++++++++++++++++++++++++++++
 3 files changed, 165 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/dummy_hcd.c b/drivers/usb/gadget/dummy_hcd.c
index e755a9d..7f6a2d8 100644
--- a/drivers/usb/gadget/dummy_hcd.c
+++ b/drivers/usb/gadget/dummy_hcd.c
@@ -1496,7 +1496,8 @@ static int handle_control_request(struct dummy_hcd *dum_hcd, struct urb *urb,
 					   Dev_InRequest) {
 					buf[0] = (u8)dum->devstatus;
 				} else
-					buf[0] = 0;
+					/* Let the function handle this */
+					break;
 			}
 			if (urb->transfer_buffer_length > 1)
 				buf[1] = 0;
diff --git a/drivers/usb/gadget/f_loopback.c b/drivers/usb/gadget/f_loopback.c
index ca660d4..75bac6d 100644
--- a/drivers/usb/gadget/f_loopback.c
+++ b/drivers/usb/gadget/f_loopback.c
@@ -427,7 +427,7 @@ int __init loopback_add(struct usb_composite_dev *cdev, bool autoresume)
 
 	/* support autoresume for remote wakeup testing */
 	if (autoresume)
-		sourcesink_driver.bmAttributes |= USB_CONFIG_ATT_WAKEUP;
+		loopback_driver.bmAttributes |= USB_CONFIG_ATT_WAKEUP;
 
 	/* support OTG systems */
 	if (gadget_is_otg(cdev->gadget)) {
diff --git a/drivers/usb/gadget/f_sourcesink.c b/drivers/usb/gadget/f_sourcesink.c
index 5247f07..5c5da19 100644
--- a/drivers/usb/gadget/f_sourcesink.c
+++ b/drivers/usb/gadget/f_sourcesink.c
@@ -59,6 +59,12 @@ struct f_sourcesink {
 
 	struct usb_ep		*in_ep;
 	struct usb_ep		*out_ep;
+
+	/* Function Power Management */
+	bool			    func_suspended;
+	bool			    func_wakeup_capable;
+	bool			    func_wakeup_enabled;
+	struct			    device dev;
 };
 
 static inline struct f_sourcesink *func_to_ss(struct usb_function *f)
@@ -191,6 +197,79 @@ static struct usb_gadget_strings *sourcesink_strings[] = {
 	NULL,
 };
 
+/*************************** DEVICE ATTRIBUTES ***************************/
+
+static ssize_t f_sourcesink_show_func_suspend(struct device *dev,
+	struct device_attribute *attr,
+	char *buf)
+{
+	struct f_sourcesink *ss = container_of(dev, struct f_sourcesink,
+		dev);
+	return sprintf(buf, "%d\n", ss->func_suspended);
+}
+
+static ssize_t f_sourcesink_show_func_wakeup_enabled(struct device *dev,
+	struct device_attribute *attr,
+	char *buf)
+{
+	struct f_sourcesink *ss = container_of(dev, struct f_sourcesink,
+		dev);
+	return sprintf(buf, "%d\n", ss->func_wakeup_enabled);
+}
+
+static ssize_t f_sourcesink_show_func_wakeup_capable(struct device *dev,
+	struct device_attribute *attr,
+	char *buf)
+{
+	struct f_sourcesink *ss = container_of(dev, struct f_sourcesink,
+		dev);
+	return sprintf(buf, "%d\n", ss->func_wakeup_capable);
+}
+
+static ssize_t f_sourcesink_store_func_wakeup_capable(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct f_sourcesink *ss = container_of(dev, struct f_sourcesink, dev);
+	unsigned long func_wakeup_capable;
+
+	/* Allows changing function wakeup capable field from the file system */
+	if (strict_strtoul(buf, 2, &func_wakeup_capable))
+		return -EINVAL;
+	ss->func_wakeup_capable = (bool)func_wakeup_capable;
+	return count;
+}
+
+static ssize_t f_sourcesink_store_func_wakeup_trigger(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct f_sourcesink *ss = container_of(dev, struct f_sourcesink, dev);
+
+	/* Allows trigerring function wakeup from the file system */
+	if (!ss->func_wakeup_capable || !ss->func_wakeup_enabled)
+		return -EINVAL;
+
+	if (usb_gadget_wakeup(ss->function.config->cdev->gadget,
+				       source_sink_intf.bInterfaceNumber) < 0)
+		return -EINVAL;
+	return count;
+}
+
+static DEVICE_ATTR(func_suspend, 0444, f_sourcesink_show_func_suspend, NULL);
+static DEVICE_ATTR(func_wakeup_enabled, 0444,
+	f_sourcesink_show_func_wakeup_enabled, NULL);
+static DEVICE_ATTR(func_wakeup_capable, 0666,
+	f_sourcesink_show_func_wakeup_capable,
+	f_sourcesink_store_func_wakeup_capable);
+static DEVICE_ATTR(func_wakeup_trigger, 0666, NULL,
+	f_sourcesink_store_func_wakeup_trigger);
+
+/*-------------------------------------------------------------------------*/
+
+static void sourcesink_release(struct device *dev)
+{
+	/* Nothing needs to be done */
+}
+
 /*-------------------------------------------------------------------------*/
 
 static int __init
@@ -199,6 +278,7 @@ sourcesink_bind(struct usb_configuration *c, struct usb_function *f)
 	struct usb_composite_dev *cdev = c->cdev;
 	struct f_sourcesink	*ss = func_to_ss(f);
 	int	id;
+	int	result = 0;
 
 	/* allocate interface ID(s) */
 	id = usb_interface_id(c, f);
@@ -243,12 +323,66 @@ autoconf_fail:
 	    (gadget_is_superspeed(c->cdev->gadget) ? "super" :
 	     (gadget_is_dualspeed(c->cdev->gadget) ? "dual" : "full")),
 			f->name, ss->in_ep->name, ss->out_ep->name);
+
+	ss->dev.parent = &cdev->gadget->dev;
+	ss->dev.release = sourcesink_release;
+	dev_set_name(&ss->dev, "sourcesink");
+
+	result = device_register(&ss->dev);
+	if (result) {
+		ERROR(cdev, "failed to register: %d\n", result);
+		goto err_device_register;
+	}
+
+	result = device_create_file(&ss->dev, &dev_attr_func_suspend);
+	if (result) {
+		ERROR(cdev, "device_create_file failed\n", result);
+		goto err_func_suspend_file;
+	}
+
+	result = device_create_file(&ss->dev, &dev_attr_func_wakeup_enabled);
+	if (result) {
+		ERROR(cdev, "device_create_file failed\n", result);
+		goto err_func_wake_enabled_file;
+	}
+
+	result = device_create_file(&ss->dev, &dev_attr_func_wakeup_capable);
+	if (result) {
+		ERROR(cdev, "device_create_file failed\n", result);
+		goto err_func_wake_capable_file;
+	}
+
+	result = device_create_file(&ss->dev, &dev_attr_func_wakeup_trigger);
+	if (result) {
+		ERROR(cdev, "device_create_file failed\n", result);
+		goto err_func_wake_trigger_file;
+	}
+
 	return 0;
+
+err_func_wake_trigger_file:
+	device_remove_file(&ss->dev, &dev_attr_func_wakeup_capable);
+err_func_wake_capable_file:
+	device_remove_file(&ss->dev, &dev_attr_func_wakeup_enabled);
+err_func_wake_enabled_file:
+	device_remove_file(&ss->dev, &dev_attr_func_suspend);
+err_func_suspend_file:
+	device_unregister(&ss->dev);
+err_device_register:
+	return -ENODEV;
 }
 
 static void
 sourcesink_unbind(struct usb_configuration *c, struct usb_function *f)
 {
+	struct f_sourcesink	*ss = func_to_ss(f);
+
+	device_remove_file(&ss->dev, &dev_attr_func_suspend);
+	device_remove_file(&ss->dev, &dev_attr_func_wakeup_capable);
+	device_remove_file(&ss->dev, &dev_attr_func_wakeup_enabled);
+	device_remove_file(&ss->dev, &dev_attr_func_wakeup_trigger);
+	device_unregister(&ss->dev);
+
 	kfree(func_to_ss(f));
 }
 
@@ -383,6 +517,32 @@ static int source_sink_start_ep(struct f_sourcesink *ss, bool is_in)
 	return status;
 }
 
+static int sourcesink_func_suspend(struct usb_function *f,
+	u8 suspend_opt)
+{
+	struct f_sourcesink *ss = func_to_ss(f);
+	struct usb_composite_dev	*cdev;
+
+	cdev = ss->function.config->cdev;
+
+	/* Parse suspend options */
+	ss->func_suspended = suspend_opt & USB_INTR_FUNC_SUSPEND_SUSP;
+	ss->func_wakeup_enabled = suspend_opt & USB_INTR_FUNC_SUSPEND_RWAKE_EN;
+
+	return 1;
+}
+
+static int sourcesink_get_status(struct usb_function *f)
+{
+	struct f_sourcesink *ss = func_to_ss(f);
+	struct usb_composite_dev	*cdev;
+
+	cdev = ss->function.config->cdev;
+
+	return ss->func_wakeup_capable << USB_INTR_STAT_RWAKE_CAP
+		| (ss->func_wakeup_enabled << USB_INTR_STAT_RWAKE_EN);
+}
+
 static void disable_source_sink(struct f_sourcesink *ss)
 {
 	struct usb_composite_dev	*cdev;
@@ -474,6 +634,8 @@ static int __init sourcesink_bind_config(struct usb_configuration *c)
 	ss->function.unbind = sourcesink_unbind;
 	ss->function.set_alt = sourcesink_set_alt;
 	ss->function.disable = sourcesink_disable;
+	ss->function.func_suspend = sourcesink_func_suspend;
+	ss->function.get_status = sourcesink_get_status;
 
 	status = usb_add_function(c, &ss->function);
 	if (status)
-- 
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.

Re: [RFC/PATCH 1/3] usb: Add SuperSpeed support to g_zero gadget

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 542 bytes --]

Hi,

On Sun, Jul 03, 2011 at 05:29:31PM +0300, Amit Blay wrote:
> This patch adds SuperSpeed descriptors to the g_zero gadget.
> The SuperSpeed descriptors were added both for f_soursesink and f_loopback
> configurations of the g_zero gadget.
> 
> Signed-off-by: Tatyana Brokhman <tlinder-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> Signed-off-by: Amit Blay <ablay-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>

applied this one alone. The others we need to dicuss a bit further. I'll
get to them after this merge window.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [RFC/PATCH 2/3] usb:gadget: Add SuperSpeed function power management

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 6376 bytes --]

Hi,

On Sun, Jul 03, 2011 at 05:29:32PM +0300, Amit Blay wrote:
> 1. Add missing definitions in ch9.h for requests tarageted to an

typo... s/tarageted/targeted

> Interface: GET_STATUS & SET_FEATURE(FUNC_SUSPEND).

if it's targeted to an interface, why don't you just let the gadget
driver handle it ? composite.c tracks all functions already and we
should just call function->setup() to let the correct function handle
this.

> 2. Add func_wakeup api to usb_gadget_ops.
> Super-Speed device must provide interface number to the UDC when
> it triggers remote wakeup (function wake).
> The func_wakeup api is used instead of the wakeup api, when the
> gadget is connected in Super-Speed. The wakeup api is left as is,
> and it is used when the gadget is connected in High-Speed. Therefore,
> no change is required in non Super-Speed UDCs.

first of all, this needs to be splitted. You shouldn't do more than one
thing in a patch ;-)

> Signed-off-by: Amit Blay <ablay-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> 
> ---
>  drivers/usb/gadget/udc-core.c |    2 +-
>  drivers/usb/gadget/zero.c     |    6 +++++-
>  include/linux/usb/ch9.h       |    8 ++++++++
>  include/linux/usb/gadget.h    |   35 +++++++++++++++++++++++++++--------
>  4 files changed, 41 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/usb/gadget/udc-core.c b/drivers/usb/gadget/udc-core.c
> index 05ba472..beb7e89 100644
> --- a/drivers/usb/gadget/udc-core.c
> +++ b/drivers/usb/gadget/udc-core.c
> @@ -347,7 +347,7 @@ static ssize_t usb_udc_srp_store(struct device *dev,
>  	struct usb_udc		*udc = dev_get_drvdata(dev);
>  
>  	if (sysfs_streq(buf, "1"))
> -		usb_gadget_wakeup(udc->gadget);
> +		usb_gadget_wakeup(udc->gadget, 0);
>  
>  	return n;
>  }
> diff --git a/drivers/usb/gadget/zero.c b/drivers/usb/gadget/zero.c
> index 00e2fd2..a5d13c6 100644
> --- a/drivers/usb/gadget/zero.c
> +++ b/drivers/usb/gadget/zero.c
> @@ -239,7 +239,11 @@ static void zero_autoresume(unsigned long _c)
>  	 * because of some direct user request.
>  	 */
>  	if (g->speed != USB_SPEED_UNKNOWN) {
> -		int status = usb_gadget_wakeup(g);
> +		/*
> +		 * The single interface of the current configuration
> +		 * triggers the wakeup.
> +		 */
> +		int status = usb_gadget_wakeup(g, 1);

no no, I think this should be handled by the function itself (f_*.c).

>  		INFO(cdev, "%s --> %d\n", __func__, status);
>  	}
>  }
> diff --git a/include/linux/usb/ch9.h b/include/linux/usb/ch9.h
> index 0fd3fbd..404c10e 100644
> --- a/include/linux/usb/ch9.h
> +++ b/include/linux/usb/ch9.h
> @@ -142,7 +142,13 @@
>  #define USB_DEVICE_LTM_ENABLE	50	/* dev may send LTM */
>  #define USB_INTRF_FUNC_SUSPEND	0	/* function suspend */
>  
> +/*
> + * Function Suspend Options as defined by USB 3.0
> + * See USB 3.0 spec Table 9-7
> + */
>  #define USB_INTR_FUNC_SUSPEND_OPT_MASK	0xFF00
> +#define USB_INTR_FUNC_SUSPEND_SUSP		1 /* function suspend option */
> +#define USB_INTR_FUNC_SUSPEND_RWAKE_EN	2 /* function wake enabled option */
>  
>  #define USB_ENDPOINT_HALT		0	/* IN/OUT will STALL */
>  
> @@ -150,6 +156,8 @@
>  #define USB_DEV_STAT_U1_ENABLED		2	/* transition into U1 state */
>  #define USB_DEV_STAT_U2_ENABLED		3	/* transition into U2 state */
>  #define USB_DEV_STAT_LTM_ENABLED	4	/* Latency tolerance messages */
> +#define USB_INTR_STAT_RWAKE_CAP		0	/* Function wake capable */
> +#define USB_INTR_STAT_RWAKE_EN			1	/* Function wake enabled */
>  
>  /**
>   * struct usb_ctrlrequest - SETUP data for a USB device control request
> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> index 087f4b9..3ebbc11 100644
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -458,7 +458,11 @@ struct usb_gadget_ops {
>  	int	(*pullup) (struct usb_gadget *, int is_on);
>  	int	(*ioctl)(struct usb_gadget *,
>  				unsigned code, unsigned long param);
> +
> +	/* USB 3.0 additions */

this comment is not part of this patch ;-) (nitpicking, I know)

>  	void	(*get_config_params)(struct usb_dcd_config_params *);
> +	int     (*func_wakeup)(struct usb_gadget *, int interface_id);
> +
>  	int	(*udc_start)(struct usb_gadget *,
>  			struct usb_gadget_driver *);
>  	int	(*udc_stop)(struct usb_gadget *,
> @@ -607,21 +611,36 @@ static inline int usb_gadget_frame_number(struct usb_gadget *gadget)
>  /**
>   * usb_gadget_wakeup - tries to wake up the host connected to this gadget
>   * @gadget: controller used to wake up the host
> + * @interface_id: id of the first interface of the function that
> + *	triggered the wake up
>   *
>   * Returns zero on success, else negative error code if the hardware
>   * doesn't support such attempts, or its support has not been enabled
>   * by the usb host.  Drivers must return device descriptors that report
>   * their ability to support this, or hosts won't enable it.
> + * For Super-Speed devices only, the gadget should provide the
> + * id of the first interface that triggered the wake up
> + * (function wake). For non Super-Speed devices, the value of
> + * this parameter doesn't matter.
>   *
> - * This may also try to use SRP to wake the host and start enumeration,
> - * even if OTG isn't otherwise in use.  OTG devices may also start
> - * remote wakeup even when hosts don't explicitly enable it.
> + * This may also try to use SRP to wake the host and start
> + * enumeration, even if OTG isn't otherwise in use.  OTG devices
> + * may also start remote wakeup even when hosts don't explicitly
> + * enable it.
>   */
> -static inline int usb_gadget_wakeup(struct usb_gadget *gadget)
> +static inline int usb_gadget_wakeup(struct usb_gadget *gadget, int interface_id)
>  {
> -	if (!gadget->ops->wakeup)
> -		return -EOPNOTSUPP;
> -	return gadget->ops->wakeup(gadget);
> +	if (gadget->speed == USB_SPEED_SUPER) {
> +		if (!gadget->ops->func_wakeup)
> +			return -EOPNOTSUPP;
> +
> +		return gadget->ops->func_wakeup(gadget, interface_id);

I really don't like this... You're just abusing this interface. Either
add a separate one (which I still don't think it's the right approach)
or just let the gadget driver handle it, meaning that composite.c would
call into f_*.c and the drivers which don't use composite.c will handle
it their own way.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [RFC/PATCH 3/3] usb:gadget: SuperSpeed function power management testing support

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 6422 bytes --]

On Sun, Jul 03, 2011 at 05:29:33PM +0300, Amit Blay wrote:
> 1. Fix dummy_hcd to let the function handle GET_STATUS(Interface) request.
> 2. Fix a bug in f_loopback Gadget which updated the bmAttribute of
> f_sourcesink instead of f_loopback.
> 3. Update f_sourcesink Gadget to support function suspend & wakeup.
> This is done by adding get_status() & func_suspend() handlers.
> The current status of the function is controlable via debug FS:
> (wakeup capable, wakeup enabled, suspended).
> 
> Signed-off-by: Amit Blay <ablay-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> 
> ---
>  drivers/usb/gadget/dummy_hcd.c    |    3 +-
>  drivers/usb/gadget/f_loopback.c   |    2 +-
>  drivers/usb/gadget/f_sourcesink.c |  162 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 165 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/gadget/dummy_hcd.c b/drivers/usb/gadget/dummy_hcd.c
> index e755a9d..7f6a2d8 100644
> --- a/drivers/usb/gadget/dummy_hcd.c
> +++ b/drivers/usb/gadget/dummy_hcd.c
> @@ -1496,7 +1496,8 @@ static int handle_control_request(struct dummy_hcd *dum_hcd, struct urb *urb,
>  					   Dev_InRequest) {
>  					buf[0] = (u8)dum->devstatus;
>  				} else
> -					buf[0] = 0;
> +					/* Let the function handle this */
> +					break;
>  			}
>  			if (urb->transfer_buffer_length > 1)
>  				buf[1] = 0;
> diff --git a/drivers/usb/gadget/f_loopback.c b/drivers/usb/gadget/f_loopback.c
> index ca660d4..75bac6d 100644
> --- a/drivers/usb/gadget/f_loopback.c
> +++ b/drivers/usb/gadget/f_loopback.c
> @@ -427,7 +427,7 @@ int __init loopback_add(struct usb_composite_dev *cdev, bool autoresume)
>  
>  	/* support autoresume for remote wakeup testing */
>  	if (autoresume)
> -		sourcesink_driver.bmAttributes |= USB_CONFIG_ATT_WAKEUP;
> +		loopback_driver.bmAttributes |= USB_CONFIG_ATT_WAKEUP;

this change doesn't belong here.

> diff --git a/drivers/usb/gadget/f_sourcesink.c b/drivers/usb/gadget/f_sourcesink.c
> index 5247f07..5c5da19 100644
> --- a/drivers/usb/gadget/f_sourcesink.c
> +++ b/drivers/usb/gadget/f_sourcesink.c
> @@ -59,6 +59,12 @@ struct f_sourcesink {
>  
>  	struct usb_ep		*in_ep;
>  	struct usb_ep		*out_ep;
> +
> +	/* Function Power Management */
> +	bool			    func_suspended;
> +	bool			    func_wakeup_capable;
> +	bool			    func_wakeup_enabled;
> +	struct			    device dev;

this should *NOT* have a device of its own. And the way you tabified is
wrong.

>  };
>  
>  static inline struct f_sourcesink *func_to_ss(struct usb_function *f)
> @@ -191,6 +197,79 @@ static struct usb_gadget_strings *sourcesink_strings[] = {
>  	NULL,
>  };
>  
> +/*************************** DEVICE ATTRIBUTES ***************************/
> +
> +static ssize_t f_sourcesink_show_func_suspend(struct device *dev,
> +	struct device_attribute *attr,
> +	char *buf)
> +{
> +	struct f_sourcesink *ss = container_of(dev, struct f_sourcesink,
> +		dev);
> +	return sprintf(buf, "%d\n", ss->func_suspended);
> +}
> +
> +static ssize_t f_sourcesink_show_func_wakeup_enabled(struct device *dev,
> +	struct device_attribute *attr,
> +	char *buf)
> +{
> +	struct f_sourcesink *ss = container_of(dev, struct f_sourcesink,
> +		dev);
> +	return sprintf(buf, "%d\n", ss->func_wakeup_enabled);
> +}
> +
> +static ssize_t f_sourcesink_show_func_wakeup_capable(struct device *dev,
> +	struct device_attribute *attr,
> +	char *buf)
> +{
> +	struct f_sourcesink *ss = container_of(dev, struct f_sourcesink,
> +		dev);
> +	return sprintf(buf, "%d\n", ss->func_wakeup_capable);
> +}
> +
> +static ssize_t f_sourcesink_store_func_wakeup_capable(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t count)
> +{
> +	struct f_sourcesink *ss = container_of(dev, struct f_sourcesink, dev);
> +	unsigned long func_wakeup_capable;
> +
> +	/* Allows changing function wakeup capable field from the file system */
> +	if (strict_strtoul(buf, 2, &func_wakeup_capable))
> +		return -EINVAL;
> +	ss->func_wakeup_capable = (bool)func_wakeup_capable;
> +	return count;
> +}
> +
> +static ssize_t f_sourcesink_store_func_wakeup_trigger(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t count)
> +{
> +	struct f_sourcesink *ss = container_of(dev, struct f_sourcesink, dev);
> +
> +	/* Allows trigerring function wakeup from the file system */
> +	if (!ss->func_wakeup_capable || !ss->func_wakeup_enabled)
> +		return -EINVAL;
> +
> +	if (usb_gadget_wakeup(ss->function.config->cdev->gadget,
> +				       source_sink_intf.bInterfaceNumber) < 0)
> +		return -EINVAL;
> +	return count;
> +}

why didn't you do this for composite.c ? Then all function drivers will
be able to be tested.

> @@ -199,6 +278,7 @@ sourcesink_bind(struct usb_configuration *c, struct usb_function *f)
>  	struct usb_composite_dev *cdev = c->cdev;
>  	struct f_sourcesink	*ss = func_to_ss(f);
>  	int	id;
> +	int	result = 0;
>  
>  	/* allocate interface ID(s) */
>  	id = usb_interface_id(c, f);
> @@ -243,12 +323,66 @@ autoconf_fail:
>  	    (gadget_is_superspeed(c->cdev->gadget) ? "super" :
>  	     (gadget_is_dualspeed(c->cdev->gadget) ? "dual" : "full")),
>  			f->name, ss->in_ep->name, ss->out_ep->name);
> +
> +	ss->dev.parent = &cdev->gadget->dev;
> +	ss->dev.release = sourcesink_release;
> +	dev_set_name(&ss->dev, "sourcesink");
> +
> +	result = device_register(&ss->dev);
> +	if (result) {
> +		ERROR(cdev, "failed to register: %d\n", result);
> +		goto err_device_register;
> +	}
> +
> +	result = device_create_file(&ss->dev, &dev_attr_func_suspend);
> +	if (result) {
> +		ERROR(cdev, "device_create_file failed\n", result);
> +		goto err_func_suspend_file;
> +	}
> +
> +	result = device_create_file(&ss->dev, &dev_attr_func_wakeup_enabled);
> +	if (result) {
> +		ERROR(cdev, "device_create_file failed\n", result);
> +		goto err_func_wake_enabled_file;
> +	}
> +
> +	result = device_create_file(&ss->dev, &dev_attr_func_wakeup_capable);
> +	if (result) {
> +		ERROR(cdev, "device_create_file failed\n", result);
> +		goto err_func_wake_capable_file;
> +	}
> +
> +	result = device_create_file(&ss->dev, &dev_attr_func_wakeup_trigger);
> +	if (result) {
> +		ERROR(cdev, "device_create_file failed\n", result);
> +		goto err_func_wake_trigger_file;
> +	}

dude ?!? Make a sysfs group ;-)

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [RFC/PATCH 2/3] usb:gadget: Add SuperSpeed function power management

[Amit Blay]

Hi Felipe,

>> Interface: GET_STATUS & SET_FEATURE(FUNC_SUSPEND).
>
> if it's targeted to an interface, why don't you just let the gadget
> driver handle it ? composite.c tracks all functions already and we
> should just call function->setup() to let the correct function handle
> this.

Your question is regarding why we added the function->func_suspend &
function->get_status callbacks in the first place? (I'm asking because
this is not covered by this patch...)
If we let both requests to be handled by function->setup(), then to
support SuperSpeed, we'll have to change each and every function's setup()
to respond back with a correct default value.
The advantage of adding function->func_suspend & function->get_status is
that if the function doesn't supply those functions, the default can be
handled by the composite setup() function.

>> 2. Add func_wakeup api to usb_gadget_ops.
>> Super-Speed device must provide interface number to the UDC when
>> it triggers remote wakeup (function wake).
>> The func_wakeup api is used instead of the wakeup api, when the
>> gadget is connected in Super-Speed. The wakeup api is left as is,
>> and it is used when the gadget is connected in High-Speed. Therefore,
>> no change is required in non Super-Speed UDCs.

> first of all, this needs to be splitted. You shouldn't do more than one
> thing in a patch ;-)

OK, I will split the patch.

>> +++ b/drivers/usb/gadget/zero.c
>> @@ -239,7 +239,11 @@ static void zero_autoresume(unsigned long _c)
>>  	 * because of some direct user request.
>>  	 */
>>  	if (g->speed != USB_SPEED_UNKNOWN) {
>> -		int status = usb_gadget_wakeup(g);
>> +		/*
>> +		 * The single interface of the current configuration
>> +		 * triggers the wakeup.
>> +		 */
>> +		int status = usb_gadget_wakeup(g, 1);
>
> no no, I think this should be handled by the function itself (f_*.c).

Yes you are right, the function should handle this. The next patch
(usb:gadget: SuperSpeed function power management testing support) adds
this exact capability to f_sourcesink. The function invokes the UDC's
func_wake callback.

But in this change above, I tried (with minimal changes) to keep the
current functionality of gadget zero's autoresume, which uses
usb_gadget_wakeup(). Since there is no device remote wakeup in SuperSpeed,
only function wake, calling usb_gadget_wakeup() has no real meaning in
non-SuperSpeed speeds.

The complete solution will be to move the autoresume feature from gadget
zero into the functions, f_sourcesink & f_loopback. This is the way wakeup
should be done in SuperSpeed, as I understand it. That means that both
functions will arm a timer on device suspend. When the timer expires, in
SuperSpeed it should call the UDC's func_wake callback. For lower speeds,
it should call usb_gadget_wakeup() to trigger device remote wakeup.
What do you think?

>> -static inline int usb_gadget_wakeup(struct usb_gadget *gadget)
>> +static inline int usb_gadget_wakeup(struct usb_gadget *gadget, int
>> interface_id)
>>  {
>> -	if (!gadget->ops->wakeup)
>> -		return -EOPNOTSUPP;
>> -	return gadget->ops->wakeup(gadget);
>> +	if (gadget->speed == USB_SPEED_SUPER) {
>> +		if (!gadget->ops->func_wakeup)
>> +			return -EOPNOTSUPP;
>> +
>> +		return gadget->ops->func_wakeup(gadget, interface_id);
>
> I really don't like this... You're just abusing this interface. Either
> add a separate one (which I still don't think it's the right approach)
> or just let the gadget driver handle it, meaning that composite.c would
> call into f_*.c and the drivers which don't use composite.c will handle
> it their own way.

This change is meant to keep usb_gadget_wakeup() API backwards compatible,
meaning that this API will still work in SuperSpeed.
Of course, if a new USB class will utilize function wake, the new function
will call gadget->ops->func_wakeup().
The solution I suggested above will address this comment as well, but the
downside is that it is not backward compatible, meaning, it requires to
change each gadget that is using usb_gadget_wakeup().

Thanks,
Amit.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [RFC/PATCH 2/3] usb:gadget: Add SuperSpeed function power management

[Amit Blay]

Hi Felipe,

>> Interface: GET_STATUS & SET_FEATURE(FUNC_SUSPEND).
>
> if it's targeted to an interface, why don't you just let the gadget
> driver handle it ? composite.c tracks all functions already and we
> should just call function->setup() to let the correct function handle
> this.

Your question is regarding why we added the function->func_suspend &
function->get_status callbacks in the first place? (I'm asking because
this is not covered by this patch...)
If we let both requests to be handled by function->setup(), then to
support SuperSpeed, we'll have to change each and every function's setup()
to respond back with a correct default value.
The advantage of adding function->func_suspend & function->get_status is
that if the function doesn't supply those functions, the default can be
handled by the composite setup() function.

>> 2. Add func_wakeup api to usb_gadget_ops.
>> Super-Speed device must provide interface number to the UDC when
>> it triggers remote wakeup (function wake).
>> The func_wakeup api is used instead of the wakeup api, when the
>> gadget is connected in Super-Speed. The wakeup api is left as is,
>> and it is used when the gadget is connected in High-Speed. Therefore,
>> no change is required in non Super-Speed UDCs.

> first of all, this needs to be splitted. You shouldn't do more than one
> thing in a patch ;-)

OK, I will split the patch.

>> +++ b/drivers/usb/gadget/zero.c
>> @@ -239,7 +239,11 @@ static void zero_autoresume(unsigned long _c)
>>  	 * because of some direct user request.
>>  	 */
>>  	if (g->speed != USB_SPEED_UNKNOWN) {
>> -		int status = usb_gadget_wakeup(g);
>> +		/*
>> +		 * The single interface of the current configuration
>> +		 * triggers the wakeup.
>> +		 */
>> +		int status = usb_gadget_wakeup(g, 1);
>
> no no, I think this should be handled by the function itself (f_*.c).

Yes you are right, the function should handle this. The next patch
(usb:gadget: SuperSpeed function power management testing support) adds
this exact capability to f_sourcesink. The function invokes the UDC's
func_wake callback.

But in this change above, I tried (with minimal changes) to keep the
current functionality of gadget zero's autoresume, which uses
usb_gadget_wakeup(). Since there is no device remote wakeup in SuperSpeed,
only function wake, calling usb_gadget_wakeup() has no real meaning in
non-SuperSpeed speeds.

The complete solution will be to move the autoresume feature from gadget
zero into the functions, f_sourcesink & f_loopback. This is the way wakeup
should be done in SuperSpeed, as I understand it. That means that both
functions will arm a timer on device suspend. When the timer expires, in
SuperSpeed it should call the UDC's func_wake callback. For lower speeds,
it should call usb_gadget_wakeup() to trigger device remote wakeup.
What do you think?

>> -static inline int usb_gadget_wakeup(struct usb_gadget *gadget)
>> +static inline int usb_gadget_wakeup(struct usb_gadget *gadget, int
>> interface_id)
>>  {
>> -	if (!gadget->ops->wakeup)
>> -		return -EOPNOTSUPP;
>> -	return gadget->ops->wakeup(gadget);
>> +	if (gadget->speed == USB_SPEED_SUPER) {
>> +		if (!gadget->ops->func_wakeup)
>> +			return -EOPNOTSUPP;
>> +
>> +		return gadget->ops->func_wakeup(gadget, interface_id);
>
> I really don't like this... You're just abusing this interface. Either
> add a separate one (which I still don't think it's the right approach)
> or just let the gadget driver handle it, meaning that composite.c would
> call into f_*.c and the drivers which don't use composite.c will handle
> it their own way.

This change is meant to keep usb_gadget_wakeup() API backwards compatible,
meaning that this API will still work in SuperSpeed.
Of course, if a new USB class will utilize function wake, the new function
will call gadget->ops->func_wakeup().
The solution I suggested above will address this comment as well, but the
downside is that it is not backward compatible, meaning, it requires to
change each gadget that is using usb_gadget_wakeup().

Thanks,
Amit.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [RFC/PATCH 2/3] usb:gadget: Add SuperSpeed function power management

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 4518 bytes --]

hi,

On Thu, Jul 28, 2011 at 09:15:46AM -0700, Amit Blay wrote:
> >> Interface: GET_STATUS & SET_FEATURE(FUNC_SUSPEND).
> >
> > if it's targeted to an interface, why don't you just let the gadget
> > driver handle it ? composite.c tracks all functions already and we
> > should just call function->setup() to let the correct function handle
> > this.
> 
> Your question is regarding why we added the function->func_suspend &
> function->get_status callbacks in the first place? (I'm asking because
> this is not covered by this patch...)

yes, that's really unnecessary.

> If we let both requests to be handled by function->setup(), then to
> support SuperSpeed, we'll have to change each and every function's setup()
> to respond back with a correct default value.
> The advantage of adding function->func_suspend & function->get_status is
> that if the function doesn't supply those functions, the default can be
> handled by the composite setup() function.

just handle as any other USB request. When it's implemented by the
gadget driver, we will handle it and return success, otherwise (namely
if f->setup() can't handle it) we stall.

> >> +++ b/drivers/usb/gadget/zero.c
> >> @@ -239,7 +239,11 @@ static void zero_autoresume(unsigned long _c)
> >>  	 * because of some direct user request.
> >>  	 */
> >>  	if (g->speed != USB_SPEED_UNKNOWN) {
> >> -		int status = usb_gadget_wakeup(g);
> >> +		/*
> >> +		 * The single interface of the current configuration
> >> +		 * triggers the wakeup.
> >> +		 */
> >> +		int status = usb_gadget_wakeup(g, 1);
> >
> > no no, I think this should be handled by the function itself (f_*.c).
> 
> Yes you are right, the function should handle this. The next patch
> (usb:gadget: SuperSpeed function power management testing support) adds
> this exact capability to f_sourcesink. The function invokes the UDC's
> func_wake callback.

not sure this is the right thing to do though.

> But in this change above, I tried (with minimal changes) to keep the
> current functionality of gadget zero's autoresume, which uses
> usb_gadget_wakeup(). Since there is no device remote wakeup in SuperSpeed,
> only function wake, calling usb_gadget_wakeup() has no real meaning in
> non-SuperSpeed speeds.
> 
> The complete solution will be to move the autoresume feature from gadget
> zero into the functions, f_sourcesink & f_loopback. This is the way wakeup

you shouldn't simply move it there. USB2 still has remote wakeup right ?

> should be done in SuperSpeed, as I understand it. That means that both
> functions will arm a timer on device suspend. When the timer expires, in
> SuperSpeed it should call the UDC's func_wake callback. For lower speeds,
> it should call usb_gadget_wakeup() to trigger device remote wakeup.
> What do you think?

not sure. To me, you should only to a remote wakeup (or function wake)
if the user wants to use the device. Otherwise, why are you waking up
the host ?

> >> -static inline int usb_gadget_wakeup(struct usb_gadget *gadget)
> >> +static inline int usb_gadget_wakeup(struct usb_gadget *gadget, int
> >> interface_id)
> >>  {
> >> -	if (!gadget->ops->wakeup)
> >> -		return -EOPNOTSUPP;
> >> -	return gadget->ops->wakeup(gadget);
> >> +	if (gadget->speed == USB_SPEED_SUPER) {
> >> +		if (!gadget->ops->func_wakeup)
> >> +			return -EOPNOTSUPP;
> >> +
> >> +		return gadget->ops->func_wakeup(gadget, interface_id);
> >
> > I really don't like this... You're just abusing this interface. Either
> > add a separate one (which I still don't think it's the right approach)
> > or just let the gadget driver handle it, meaning that composite.c would
> > call into f_*.c and the drivers which don't use composite.c will handle
> > it their own way.
> 
> This change is meant to keep usb_gadget_wakeup() API backwards compatible,
> meaning that this API will still work in SuperSpeed.
> Of course, if a new USB class will utilize function wake, the new function
> will call gadget->ops->func_wakeup().

then change it correctly, don't just hack around it ;-)

> The solution I suggested above will address this comment as well, but the
> downside is that it is not backward compatible, meaning, it requires to
> change each gadget that is using usb_gadget_wakeup().

so ? It won't break anything if you do it right. Only USB3-capable
gadget drivers have function wakeup... so start with the functions which
have USB3 descriptors...

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [RFC/PATCH 2/3] usb:gadget: Add SuperSpeed function power management

[Amit Blay]

Hi Felipe,

>> Your question is regarding why we added the function->func_suspend &
>> function->get_status callbacks in the first place? (I'm asking because
>> this is not covered by this patch...)
>
> yes, that's really unnecessary.
>
>> If we let both requests to be handled by function->setup(), then to
>> support SuperSpeed, we'll have to change each and every function's
>> setup()
>> to respond back with a correct default value.
>> The advantage of adding function->func_suspend & function->get_status is
>> that if the function doesn't supply those functions, the default can be
>> handled by the composite setup() function.
>
> just handle as any other USB request. When it's implemented by the
> gadget driver, we will handle it and return success, otherwise (namely
> if f->setup() can't handle it) we stall.

Then, you will not be able to support SuperSpeed without changing each
function to handle those requests in f->setup(). Some hosts may request to
get a status of an interface during enumeration, and the device will
stall.
I understand your point, but I still think that it makes sense to have
both callbacks, and have a default handling in composite.c. The
composite.c is a right place for implementing common ch9 logic, which you
will not want to re-implement for each function, and I think that this is
the case.

>> Yes you are right, the function should handle this. The next patch
>> (usb:gadget: SuperSpeed function power management testing support) adds
>> this exact capability to f_sourcesink. The function invokes the UDC's
>> func_wake callback.
>
> not sure this is the right thing to do though.

Can you please elaborate, what is not the right thing? ;-)
Are you referring to your comments for the other patch?

>> But in this change above, I tried (with minimal changes) to keep the
>> current functionality of gadget zero's autoresume, which uses
>> usb_gadget_wakeup(). Since there is no device remote wakeup in
>> SuperSpeed,
>> only function wake, calling usb_gadget_wakeup() has no real meaning in
>> non-SuperSpeed speeds.
>>
>> The complete solution will be to move the autoresume feature from gadget
>> zero into the functions, f_sourcesink & f_loopback. This is the way
>> wakeup
>
> you shouldn't simply move it there. USB2 still has remote wakeup right ?

Yes, USB2 remote wakeup will work, it will be triggered by a function
instead of by the gadget, but it will still send a USB2 device remote
wakeup to the host. (this is only for gadget zero, which is the only
gadget that seems to be using usb_gadget_wakeup for remote wakeup).

>> should be done in SuperSpeed, as I understand it. That means that both
>> functions will arm a timer on device suspend. When the timer expires, in
>> SuperSpeed it should call the UDC's func_wake callback. For lower
>> speeds,
>> it should call usb_gadget_wakeup to trigger device remote wakeup.
>> What do you think?
>
> not sure. To me, you should only to a remote wakeup (or function wake)
> if the user wants to use the device. Otherwise, why are you waking up
> the host ?

No, no. This is the same mechanism that is *already* implemented by the
zero gadget alone ("autoresume"). I just described a way we can keep this
working for USB3 (SuperSpeed and lower Speeds). The same logic could be
implemented in the two functions, and not by the zero gadget, which really
makes sense for SuperSpeed, and I think that in g_zero case, it makes
sense for USB2 as well (see my previous comment).

>> This change is meant to keep usb_gadget_wakeup() API backwards
>> compatible,
>> meaning that this API will still work in SuperSpeed.
>> Of course, if a new USB class will utilize function wake, the new
>> function
>> will call gadget->ops->func_wakeup().
>
> then change it correctly, don't just hack around it ;-)
>
>> The solution I suggested above will address this comment as well, but
>> the
>> downside is that it is not backward compatible, meaning, it requires to
>> change each gadget that is using usb_gadget_wakeup().
>
> so ? It won't break anything if you do it right. Only USB3-capable
> gadget drivers have function wakeup... so start with the functions which
> have USB3 descriptors...

I agree. The first gadget is g_zero. So please see my previous comments
discussing how g_zero and it's functions can be modified to keep it's
remote wakeup working for USB3.

Thanks Felipe for your comments,
Amit.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html