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

[Felipe Balbi <balbi@ti.com>]

[-- 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 --]