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

["Amit Blay" <ablay@codeaurora.org>]

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.