diff mbox series

[3/5] usb: dwc3: Add function suspend and function wakeup support

Message ID 1659467920-9095-4-git-send-email-quic_eserrao@quicinc.com (mailing list archive)
State New, archived
Headers show
Series Add function suspend/resume and remote wakeup support | expand

Commit Message

Elson Roy Serrao Aug. 2, 2022, 7:18 p.m. UTC 
USB host sends function suspend and resume notifications to
device through SET_FEATURE setup packet. This packet is directed
to the interface to which function suspend/resume is intended to.
Add support to handle this packet by delegating the request to composite
layer. To exit from function suspend the interface needs to trigger a
function wakeup in case it wants to initate a data transfer. Expose a
new gadget op so that an interface can send function wakeup request to
the host.

Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
---
 drivers/usb/dwc3/core.h   |  1 +
 drivers/usb/dwc3/ep0.c    | 12 ++++--------
 drivers/usb/dwc3/gadget.c | 30 ++++++++++++++++++++++++++++++
 3 files changed, 35 insertions(+), 8 deletions(-)

Comments

Thinh Nguyen Aug. 2, 2022, 11:44 p.m. UTC | #1 
On 8/2/2022, Elson Roy Serrao wrote:
> USB host sends function suspend and resume notifications to
> device through SET_FEATURE setup packet. This packet is directed
> to the interface to which function suspend/resume is intended to.
> Add support to handle this packet by delegating the request to composite
> layer. To exit from function suspend the interface needs to trigger a
> function wakeup in case it wants to initate a data transfer. Expose a
> new gadget op so that an interface can send function wakeup request to
> the host.
>
> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
> ---
>   drivers/usb/dwc3/core.h   |  1 +
>   drivers/usb/dwc3/ep0.c    | 12 ++++--------
>   drivers/usb/dwc3/gadget.c | 30 ++++++++++++++++++++++++++++++
>   3 files changed, 35 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 3306b1c..e08e522 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -519,6 +519,7 @@
>   #define DWC3_DGCMD_SET_SCRATCHPAD_ADDR_LO	0x04
>   #define DWC3_DGCMD_SET_SCRATCHPAD_ADDR_HI	0x05
>   
> +#define DWC3_DGCMD_XMIT_DEV		0x07

This is device notification, so just name this to 
DWC_DGCMD_DEV_NOTIFICATION. Also update debug.h to print the proper 
string for this command.

>   #define DWC3_DGCMD_SELECTED_FIFO_FLUSH	0x09
>   #define DWC3_DGCMD_ALL_FIFO_FLUSH	0x0a
>   #define DWC3_DGCMD_SET_ENDPOINT_NRDY	0x0c
> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
> index 4cc3d3a..cedc890 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)
> @@ -365,7 +367,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);

Why do we need to delegate to gadget driver?

We need to check if the host arms the remote wakeup, and we need to 
check if the gadget is capable of remote wakeup. For example, it's odd 
for mass_storage device to be remote capable.

So we need a flag in usb_gadget for that. Have dwc3 check against that 
flag before setting it.

Something like this:

if (dwc->gadget->rw_capable) {
     usb_status = USB_INTRF_STAT_FUNC_RW_CAP;
     if (dwc->rw_armed)
         usb_status |= USB_INTERF_STAT_FUNC_RW;
}


>   
>   	case USB_RECIP_ENDPOINT:
>   		dep = dwc3_wIndex_to_dep(dwc, ctrl->wIndex);
> @@ -511,13 +513,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);

Again, why are we delegating this to the gadget driver? This can be done 
here as something like this:

if (dwc->gadget->rw_capable && set &&
      (wIndex & USB_INTRF_FUNC_SUSPEND_RW))
         dwc->rw_armed = 1;
else
         dwc->rw_armed = 0;

dwc->rw_selected_interface = wIndex & 0xff;

>   		break;
>   	default:
>   		ret = -EINVAL;
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index d6697da..0b2947e 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2357,6 +2357,35 @@ static int dwc3_gadget_wakeup(struct usb_gadget *g)
>   	return ret;
>   }
>   
> +static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int interface_id)

Don't pass this work to the gadget driver. The upperlayer doesn't know 
when to send the function wakeup. The dwc3 driver should send the 
function wakeup when the device is in the state U0, and the dwc3 driver 
gets that info first hand.

> +{
> +	int	ret = 0;
> +	u32	reg;
> +	struct	dwc3 *dwc = gadget_to_dwc(g);
> +
> +	reg = dwc3_readl(dwc->regs, DWC3_DSTS);
> +
> +	/*
> +	 * If the link is in LPM, first bring the link to U0
> +	 * before triggering function wakeup. Ideally this
> +	 * needs to be expanded to other LPMs as well in
> +	 * addition to U3
> +	 */
> +	if (DWC3_DSTS_USBLNKST(reg) == DWC3_LINK_STATE_U3) {

You're not doing U0 link check here.

> +		dwc3_gadget_wakeup(g);
> +		return -EAGAIN;
> +	}
> +
> +	ret = dwc3_send_gadget_generic_command(dwc, DWC3_DGCMD_XMIT_DEV,
> +					       0x1 | (interface_id << 4));

The interface_id here is the dwc->rw_selected_interface as noted earlier.

> +
> +	if (ret)
> +		dev_err(dwc->dev, "Function wakeup HW command failed, ret %d\n",
> +			ret);
> +
> +	return ret;
> +}
> +
>   static int dwc3_gadget_set_selfpowered(struct usb_gadget *g,
>   		int is_selfpowered)
>   {
> @@ -2978,6 +3007,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,

BR,
Thinh
diff mbox series

Patch

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 3306b1c..e08e522 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -519,6 +519,7 @@ 
 #define DWC3_DGCMD_SET_SCRATCHPAD_ADDR_LO	0x04
 #define DWC3_DGCMD_SET_SCRATCHPAD_ADDR_HI	0x05
 
+#define DWC3_DGCMD_XMIT_DEV		0x07
 #define DWC3_DGCMD_SELECTED_FIFO_FLUSH	0x09
 #define DWC3_DGCMD_ALL_FIFO_FLUSH	0x0a
 #define DWC3_DGCMD_SET_ENDPOINT_NRDY	0x0c
diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
index 4cc3d3a..cedc890 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)
@@ -365,7 +367,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);
@@ -511,13 +513,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 d6697da..0b2947e 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2357,6 +2357,35 @@  static int dwc3_gadget_wakeup(struct usb_gadget *g)
 	return ret;
 }
 
+static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int interface_id)
+{
+	int	ret = 0;
+	u32	reg;
+	struct	dwc3 *dwc = gadget_to_dwc(g);
+
+	reg = dwc3_readl(dwc->regs, DWC3_DSTS);
+
+	/*
+	 * If the link is in LPM, first bring the link to U0
+	 * before triggering function wakeup. Ideally this
+	 * needs to be expanded to other LPMs as well in
+	 * addition to U3
+	 */
+	if (DWC3_DSTS_USBLNKST(reg) == DWC3_LINK_STATE_U3) {
+		dwc3_gadget_wakeup(g);
+		return -EAGAIN;
+	}
+
+	ret = dwc3_send_gadget_generic_command(dwc, DWC3_DGCMD_XMIT_DEV,
+					       0x1 | (interface_id << 4));
+
+	if (ret)
+		dev_err(dwc->dev, "Function wakeup HW command failed, ret %d\n",
+			ret);
+
+	return ret;
+}
+
 static int dwc3_gadget_set_selfpowered(struct usb_gadget *g,
 		int is_selfpowered)
 {
@@ -2978,6 +3007,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,