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

[Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>]

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