Re: [PATCH v2 4/5] usb: dwc3: Add function suspend and function wakeup support

[Thinh Nguyen]

On Tue, Jan 17, 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@xxxxxxxxxxx>
> ---
>  drivers/usb/dwc3/core.h   |  1 +
>  drivers/usb/dwc3/debug.h  |  2 ++
>  drivers/usb/dwc3/ep0.c    | 12 ++++--------
>  drivers/usb/dwc3/gadget.c | 36 +++++++++++++++++++++++++++++++++++-
>  4 files changed, 42 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 53ded08..5cda645 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)
> diff --git a/drivers/usb/dwc3/debug.h b/drivers/usb/dwc3/debug.h
> index 48b44b8..0897d9d 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 0c1203d..e05c2b9 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)
> @@ -367,7 +369,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 db4b438..1c958c4 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2346,7 +2346,7 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc, bool async)
>  		return 0;
>  
>  	/* poll until Link State changes to ON */
> -	retries = 20000;
> +	retries = 30000;

This seems like a fix that needs to be splitted to a separate change.
read_poll_timeout_atomic() would be useful here (as a separate/unrelated
change for status polling).

>  
>  	while (retries--) {
>  		reg = dwc3_readl(dwc->regs, DWC3_DSTS);
> @@ -2383,6 +2383,39 @@ 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 interface_id)
> +{
> +	struct  dwc3		*dwc = gadget_to_dwc(g);
> +	unsigned long		flags;
> +	int			ret;
> +
> +	spin_lock_irqsave(&dwc->lock, flags);
> +	/*
> +	 * If the link is in LPM, first bring the link to U0

Reword LMP -> low power.

> +	 * before triggering function remote wakeup.
> +	 */
> +	if (dwc->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,
> +					       0x1 | (interface_id << 4));

Create a macro for 0x1. Something like DWC3_DGCMDPAR_DN_FUNC_WAKE. Also,
create an macro for interface selection. Maybe
DWC3_DGCMDPAR_INTF_SEL(interface_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_selfpowered(struct usb_gadget *g,
>  		int is_selfpowered)
>  {
> @@ -3012,6 +3045,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_selfpowered	= dwc3_gadget_set_selfpowered,
>  	.pullup			= dwc3_gadget_pullup,
>  	.udc_start		= dwc3_gadget_start,
> -- 
> 2.7.4
> 

Thanks,
Thinh