mbox series

[v4,0/5] Add function suspend/resume and remote wakeup support

Message ID 1676316676-28377-1-git-send-email-quic_eserrao@quicinc.com
Headers show
Series Add function suspend/resume and remote wakeup support | expand

Message

Elson Roy Serrao Feb. 13, 2023, 7:31 p.m. UTC 
Changes in v4
 - Moved the wakeup bit check to bind function for warning the user at an early
   stage itself.
 - Added the remote wakeup configured check to gadget_wakeup() and func_wakeup()
   routines so that wakeup can be triggered only if user has configured it.
 - Cosmetic changes with respect to renaming the variables to reflect the operation
   better.

Changes in v3
 - Modified rw_capable flag to reflect the gadgets capability for wakeup
   signalling.
 - Added a check to configure wakeup bit in bmAttributes only if gadget
   is capable of triggering wakeup.
 - Implemented a gadget op for composite layer to inform UDC whether device
   is configured for remote wakeup.
 - Added a check in __usb_gadget_wakeup() API to trigger wakeup only if the
   device is configured for it.
 - Cosmetic changes in dwc3_gadget_func_wakeup() API.

Changes in v2
 - Added a flag to indicate whether the device is remote wakeup capable.
 - Added an async parameter to _dwc3_gadget_wakeup() API and few cosmetic
   changes.
 - Added flags to reflect the state of  function suspend and function remote
   wakeup to usb_function struct rather than function specific struct (f_ecm).
 - Changed the dwc3_gadget_func__wakeup() API to run synchronously by first
   checking the link state and then sending the device notification. Also
   added debug log for DEVICE_NOTIFICATION generic cmd.
 - Added changes to arm the device for remotewakeup/function remotewakeup
   only if device is capable.

An usb device can initate a remote wakeup and bring the link out of
suspend as dictated by the DEVICE_REMOTE_WAKEUP feature selector.
To achieve this an interface can invoke gadget_wakeup op and wait for the
device to come out of LPM. But the current polling based implementation
fails if the host takes a long time to drive the resume signaling specially
in high speed capable devices. Switching to an interrupt based approach is
more robust and efficient. This can be leveraged by enabling link status
change events and triggering a gadget resume when the link comes to active
state.

If the device is enhanced super-speed capable, individual interfaces can
also be put into suspend state. An interface can be in function suspend
state even when the device is not in suspend state. Function suspend state
is retained throughout the device suspend entry and exit process.
A function can be put to function suspend through FUNCTION_SUSPEND feature
selector sent by the host. This setup packet also decides whether that
function is capable of initiating a function remote wakeup. When the
function sends a wakeup notification to the host the link must be first
brought to a non-U0 state and then this notification is sent.

This change adds the infrastructure needed to support the above
functionalities.

Elson Roy Serrao (5):
  usb: gadget: Properly configure the device for remote wakeup
  usb: dwc3: Add remote wakeup handling
  usb: gadget: Add function wakeup support
  usb: dwc3: Add function suspend and function wakeup support
  usb: gadget: f_ecm: Add suspend/resume and remote wakeup support

 drivers/usb/dwc3/core.h               |   5 ++
 drivers/usb/dwc3/debug.h              |   2 +
 drivers/usb/dwc3/ep0.c                |  16 ++---
 drivers/usb/dwc3/gadget.c             | 116 ++++++++++++++++++++++++++++++++--
 drivers/usb/gadget/composite.c        |  42 ++++++++++++
 drivers/usb/gadget/configfs.c         |   3 +
 drivers/usb/gadget/function/f_ecm.c   |  68 ++++++++++++++++++++
 drivers/usb/gadget/function/u_ether.c |  63 ++++++++++++++++++
 drivers/usb/gadget/function/u_ether.h |   4 ++
 drivers/usb/gadget/udc/core.c         |  48 ++++++++++++++
 drivers/usb/gadget/udc/trace.h        |   5 ++
 include/linux/usb/composite.h         |   8 +++
 include/linux/usb/gadget.h            |  12 ++++
 13 files changed, 378 insertions(+), 14 deletions(-)

Comments

Thinh Nguyen Feb. 15, 2023, 12:55 a.m. UTC | #1 
On Mon, Feb 13, 2023, Elson Roy Serrao wrote:
> A function which is in function suspend state has to send a
> function wake notification to the host in case it needs to
> exit from this state and resume data transfer. Add support to
> handle such requests by exposing a new gadget op.
> 
> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
> ---
>  drivers/usb/gadget/composite.c | 24 ++++++++++++++++++++++++
>  drivers/usb/gadget/udc/core.c  | 21 +++++++++++++++++++++
>  include/linux/usb/composite.h  |  6 ++++++
>  include/linux/usb/gadget.h     |  4 ++++
>  4 files changed, 55 insertions(+)
> 
> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> index a37a8f4..51d6ee9 100644
> --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -492,6 +492,30 @@ int usb_interface_id(struct usb_configuration *config,
>  }
>  EXPORT_SYMBOL_GPL(usb_interface_id);
>  
> +int usb_func_wakeup(struct usb_function *func)
> +{
> +	int ret, id;
> +
> +	if (!func->func_wakeup_armed) {
> +		ERROR(func->config->cdev, "not armed for func remote wakeup\n");
> +		return -EINVAL;
> +	}
> +
> +	for (id = 0; id < MAX_CONFIG_INTERFACES; id++)
> +		if (func->config->interface[id] == func)
> +			break;
> +
> +	if (id == MAX_CONFIG_INTERFACES) {
> +		ERROR(func->config->cdev, "Invalid function id:%d\n", id);

The print of id here is always MAX_CONFIG_INTERFACES right?

Thanks,
Thinh

> +		return -EINVAL;
> +	}
> +
> +	ret = usb_gadget_func_wakeup(func->config->cdev->gadget, id);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(usb_func_wakeup);
> +
Thinh Nguyen Feb. 15, 2023, 1:19 a.m. UTC | #2 
On Mon, Feb 13, 2023, Elson Roy Serrao wrote:
> USB host sends function suspend and function resume notifications to
> the interface through SET_FEATURE/CLEAR_FEATURE setup packets.
> Add support to handle these packets by delegating the requests to
> composite layer. Also add support to handle function wake notification
> requests to exit from function suspend state.
> 
> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
> ---
>  drivers/usb/dwc3/core.h   |  3 +++
>  drivers/usb/dwc3/debug.h  |  2 ++
>  drivers/usb/dwc3/ep0.c    | 12 ++++--------
>  drivers/usb/dwc3/gadget.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 51 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index cc78236..1565b21 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -526,6 +526,7 @@
>  #define DWC3_DGCMD_SET_ENDPOINT_NRDY	0x0c
>  #define DWC3_DGCMD_SET_ENDPOINT_PRIME	0x0d
>  #define DWC3_DGCMD_RUN_SOC_BUS_LOOPBACK	0x10
> +#define DWC3_DGCMD_DEV_NOTIFICATION	0x07
>  
>  #define DWC3_DGCMD_STATUS(n)		(((n) >> 12) & 0x0F)
>  #define DWC3_DGCMD_CMDACT		BIT(10)
> @@ -538,6 +539,8 @@
>  #define DWC3_DGCMDPAR_TX_FIFO			BIT(5)
>  #define DWC3_DGCMDPAR_LOOPBACK_DIS		(0 << 0)
>  #define DWC3_DGCMDPAR_LOOPBACK_ENA		BIT(0)
> +#define DWC3_DGCMDPAR_DN_FUNC_WAKE		BIT(0)
> +#define DWC3_DGCMDPAR_INTF_SEL(n)		((n) << 4)
>  
>  /* Device Endpoint Command Register */
>  #define DWC3_DEPCMD_PARAM_SHIFT		16
> diff --git a/drivers/usb/dwc3/debug.h b/drivers/usb/dwc3/debug.h
> index 8bb2c9e..09d7038 100644
> --- a/drivers/usb/dwc3/debug.h
> +++ b/drivers/usb/dwc3/debug.h
> @@ -72,6 +72,8 @@ dwc3_gadget_generic_cmd_string(u8 cmd)
>  		return "Set Endpoint Prime";
>  	case DWC3_DGCMD_RUN_SOC_BUS_LOOPBACK:
>  		return "Run SoC Bus Loopback Test";
> +	case DWC3_DGCMD_DEV_NOTIFICATION:
> +		return "Device Notification";
>  	default:
>  		return "UNKNOWN";
>  	}
> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
> index f90fa55..1e7cc15 100644
> --- a/drivers/usb/dwc3/ep0.c
> +++ b/drivers/usb/dwc3/ep0.c
> @@ -30,6 +30,8 @@
>  static void __dwc3_ep0_do_control_status(struct dwc3 *dwc, struct dwc3_ep *dep);
>  static void __dwc3_ep0_do_control_data(struct dwc3 *dwc,
>  		struct dwc3_ep *dep, struct dwc3_request *req);
> +static int dwc3_ep0_delegate_req(struct dwc3 *dwc,
> +				 struct usb_ctrlrequest *ctrl);
>  
>  static void dwc3_ep0_prepare_one_trb(struct dwc3_ep *dep,
>  		dma_addr_t buf_dma, u32 len, u32 type, bool chain)
> @@ -368,7 +370,7 @@ static int dwc3_ep0_handle_status(struct dwc3 *dwc,
>  		 * Function Remote Wake Capable	D0
>  		 * Function Remote Wakeup	D1
>  		 */
> -		break;
> +		return dwc3_ep0_delegate_req(dwc, ctrl);
>  
>  	case USB_RECIP_ENDPOINT:
>  		dep = dwc3_wIndex_to_dep(dwc, ctrl->wIndex);
> @@ -514,13 +516,7 @@ static int dwc3_ep0_handle_intf(struct dwc3 *dwc,
>  
>  	switch (wValue) {
>  	case USB_INTRF_FUNC_SUSPEND:
> -		/*
> -		 * REVISIT: Ideally we would enable some low power mode here,
> -		 * however it's unclear what we should be doing here.
> -		 *
> -		 * For now, we're not doing anything, just making sure we return
> -		 * 0 so USB Command Verifier tests pass without any errors.
> -		 */
> +		ret = dwc3_ep0_delegate_req(dwc, ctrl);
>  		break;
>  	default:
>  		ret = -EINVAL;
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index b7fef5d..9905875 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2391,6 +2391,47 @@ static int dwc3_gadget_wakeup(struct usb_gadget *g)
>  	return ret;
>  }
>  
> +static void dwc3_resume_gadget(struct dwc3 *dwc);
> +
> +static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int intf_id)
> +{
> +	struct  dwc3		*dwc = gadget_to_dwc(g);
> +	unsigned long		flags;
> +	int			ret;
> +	int			link_state;
> +
> +	if (!dwc->wakeup_configured) {
> +		dev_err(dwc->dev, "remote wakeup not configured\n");
> +		return -EINVAL;
> +	}
> +
> +	spin_lock_irqsave(&dwc->lock, flags);
> +	/*
> +	 * If the link is in low power, first bring the link to U0
> +	 * before triggering function remote wakeup.

This should be reword as below:
If the link is in U3, signal for remote wakeup and wait for link to
transition to U0 before sending device notification.

After the above change, you can add my ack:

Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>

Thanks,
Thinh

> +	 */
> +	link_state = dwc3_gadget_get_link_state(dwc);
> +	if (link_state == DWC3_LINK_STATE_U3) {
> +		ret = __dwc3_gadget_wakeup(dwc, false);
> +		if (ret) {
> +			spin_unlock_irqrestore(&dwc->lock, flags);
> +			return -EINVAL;
> +		}
> +		dwc3_resume_gadget(dwc);
> +		dwc->link_state = DWC3_LINK_STATE_U0;
> +	}
> +
> +	ret = dwc3_send_gadget_generic_command(dwc, DWC3_DGCMD_DEV_NOTIFICATION,
> +					       DWC3_DGCMDPAR_DN_FUNC_WAKE |
> +					       DWC3_DGCMDPAR_INTF_SEL(intf_id));
> +	if (ret)
> +		dev_err(dwc->dev, "function remote wakeup failed, ret:%d\n", ret);
> +
> +	spin_unlock_irqrestore(&dwc->lock, flags);
> +
> +	return ret;
> +}
> +
>  static int dwc3_gadget_set_remote_wakeup(struct usb_gadget *g, int set)
>  {
>  	struct dwc3		*dwc = gadget_to_dwc(g);
> @@ -3028,6 +3069,7 @@ static void dwc3_gadget_async_callbacks(struct usb_gadget *g, bool enable)
>  static const struct usb_gadget_ops dwc3_gadget_ops = {
>  	.get_frame		= dwc3_gadget_get_frame,
>  	.wakeup			= dwc3_gadget_wakeup,
> +	.func_wakeup		= dwc3_gadget_func_wakeup,
>  	.set_remote_wakeup	= dwc3_gadget_set_remote_wakeup,
>  	.set_selfpowered	= dwc3_gadget_set_selfpowered,
>  	.pullup			= dwc3_gadget_pullup,
> -- 
> 2.7.4
>
Elson Roy Serrao Feb. 15, 2023, 2:10 a.m. UTC | #3 
On 2/14/2023 4:55 PM, Thinh Nguyen wrote:
> On Mon, Feb 13, 2023, Elson Roy Serrao wrote:
>> A function which is in function suspend state has to send a
>> function wake notification to the host in case it needs to
>> exit from this state and resume data transfer. Add support to
>> handle such requests by exposing a new gadget op.
>>
>> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
>> ---
>>   drivers/usb/gadget/composite.c | 24 ++++++++++++++++++++++++
>>   drivers/usb/gadget/udc/core.c  | 21 +++++++++++++++++++++
>>   include/linux/usb/composite.h  |  6 ++++++
>>   include/linux/usb/gadget.h     |  4 ++++
>>   4 files changed, 55 insertions(+)
>>
>> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
>> index a37a8f4..51d6ee9 100644
>> --- a/drivers/usb/gadget/composite.c
>> +++ b/drivers/usb/gadget/composite.c
>> @@ -492,6 +492,30 @@ int usb_interface_id(struct usb_configuration *config,
>>   }
>>   EXPORT_SYMBOL_GPL(usb_interface_id);
>>   
>> +int usb_func_wakeup(struct usb_function *func)
>> +{
>> +	int ret, id;
>> +
>> +	if (!func->func_wakeup_armed) {
>> +		ERROR(func->config->cdev, "not armed for func remote wakeup\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	for (id = 0; id < MAX_CONFIG_INTERFACES; id++)
>> +		if (func->config->interface[id] == func)
>> +			break;
>> +
>> +	if (id == MAX_CONFIG_INTERFACES) {
>> +		ERROR(func->config->cdev, "Invalid function id:%d\n", id);
> 
> The print of id here is always MAX_CONFIG_INTERFACES right?
> 
> Thanks,
> Thinh
> 
Thanks for catching this. Will rectify this in v5

>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = usb_gadget_func_wakeup(func->config->cdev->gadget, id);
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(usb_func_wakeup);
>> +