Re: [RFC/PATCH 2/3] usb:gadget: Add SuperSpeed function power management
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-arm-msm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 2/3] usb:gadget: Add SuperSpeed function power management
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.
--
To unsubscribe from this list: send the line unsubscribe linux-arm-msm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 2/3] usb:gadget: Add SuperSpeed function power management
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 ab...@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);
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_ENABLE50 /* 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_HALT0 /* 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
