2023-02-13 19:31:39

by Elson Serrao

[permalink] [raw]
Subject: [PATCH v4 0/5] Add function suspend/resume and remote wakeup support

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(-)

--
2.7.4



2023-02-13 19:31:42

by Elson Serrao

[permalink] [raw]
Subject: [PATCH v4 4/5] usb: dwc3: Add function suspend and function wakeup support

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 <[email protected]>
---
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.
+ */
+ 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


2023-02-13 19:31:44

by Elson Serrao

[permalink] [raw]
Subject: [PATCH v4 2/5] usb: dwc3: Add remote wakeup handling

An usb device can initate a remote wakeup and bring the link out of
suspend as dictated by the DEVICE_REMOTE_WAKEUP feature selector.
Add support to handle this packet and set the remote wakeup capability.

Some hosts may take longer time to initiate the resume signaling after
device triggers a remote wakeup. So add async support to the wakeup API
by enabling link status change events.

Signed-off-by: Elson Roy Serrao <[email protected]>
---
drivers/usb/dwc3/core.h | 2 ++
drivers/usb/dwc3/ep0.c | 4 +++
drivers/usb/dwc3/gadget.c | 74 +++++++++++++++++++++++++++++++++++++++++++----
3 files changed, 74 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 582ebd9..cc78236 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -1110,6 +1110,7 @@ struct dwc3_scratchpad_array {
* 3 - Reserved
* @dis_metastability_quirk: set to disable metastability quirk.
* @dis_split_quirk: set to disable split boundary.
+ * @wakeup_configured: set if the device is configured for remote wakeup.
* @imod_interval: set the interrupt moderation interval in 250ns
* increments or 0 to disable.
* @max_cfg_eps: current max number of IN eps used across all USB configs.
@@ -1327,6 +1328,7 @@ struct dwc3 {

unsigned dis_split_quirk:1;
unsigned async_callbacks:1;
+ unsigned wakeup_configured:1;

u16 imod_interval;

diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
index 61de693..f90fa55 100644
--- a/drivers/usb/dwc3/ep0.c
+++ b/drivers/usb/dwc3/ep0.c
@@ -356,6 +356,9 @@ static int dwc3_ep0_handle_status(struct dwc3 *dwc,
usb_status |= 1 << USB_DEV_STAT_U1_ENABLED;
if (reg & DWC3_DCTL_INITU2ENA)
usb_status |= 1 << USB_DEV_STAT_U2_ENABLED;
+ } else {
+ usb_status |= dwc->gadget->wakeup_armed <<
+ USB_DEVICE_REMOTE_WAKEUP;
}

break;
@@ -476,6 +479,7 @@ static int dwc3_ep0_handle_device(struct dwc3 *dwc,

switch (wValue) {
case USB_DEVICE_REMOTE_WAKEUP:
+ dwc->gadget->wakeup_armed = set;
break;
/*
* 9.4.1 says only for SS, in AddressState only for
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 3c63fa9..b7fef5d 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -258,7 +258,7 @@ int dwc3_send_gadget_generic_command(struct dwc3 *dwc, unsigned int cmd,
return ret;
}

-static int __dwc3_gadget_wakeup(struct dwc3 *dwc);
+static int __dwc3_gadget_wakeup(struct dwc3 *dwc, bool async);

/**
* dwc3_send_gadget_ep_cmd - issue an endpoint command
@@ -325,7 +325,7 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned int cmd,

fallthrough;
case DWC3_LINK_STATE_U3:
- ret = __dwc3_gadget_wakeup(dwc);
+ ret = __dwc3_gadget_wakeup(dwc, false);
dev_WARN_ONCE(dwc->dev, ret, "wakeup failed --> %d\n",
ret);
break;
@@ -2269,6 +2269,22 @@ static const struct usb_ep_ops dwc3_gadget_ep_ops = {

/* -------------------------------------------------------------------------- */

+static void dwc3_gadget_enable_linksts_evts(struct dwc3 *dwc, bool set)
+{
+ u32 reg;
+
+ if (DWC3_VER_IS_PRIOR(DWC3, 250A))
+ return;
+
+ reg = dwc3_readl(dwc->regs, DWC3_DEVTEN);
+ if (set)
+ reg |= DWC3_DEVTEN_ULSTCNGEN;
+ else
+ reg &= ~DWC3_DEVTEN_ULSTCNGEN;
+
+ dwc3_writel(dwc->regs, DWC3_DEVTEN, reg);
+}
+
static int dwc3_gadget_get_frame(struct usb_gadget *g)
{
struct dwc3 *dwc = gadget_to_dwc(g);
@@ -2276,7 +2292,7 @@ static int dwc3_gadget_get_frame(struct usb_gadget *g)
return __dwc3_gadget_get_frame(dwc);
}

-static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
+static int __dwc3_gadget_wakeup(struct dwc3 *dwc, bool async)
{
int retries;

@@ -2307,9 +2323,13 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
return -EINVAL;
}

+ if (async)
+ dwc3_gadget_enable_linksts_evts(dwc, true);
+
ret = dwc3_gadget_set_link_state(dwc, DWC3_LINK_STATE_RECOV);
if (ret < 0) {
dev_err(dwc->dev, "failed to put link in Recovery\n");
+ dwc3_gadget_enable_linksts_evts(dwc, false);
return ret;
}

@@ -2321,6 +2341,13 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
dwc3_writel(dwc->regs, DWC3_DCTL, reg);
}

+ /*
+ * Since link status change events are enabled we will receive
+ * an U0 event when wakeup is successful. So bail out.
+ */
+ if (async)
+ return 0;
+
/* poll until Link State changes to ON */
retries = 20000;

@@ -2346,13 +2373,36 @@ static int dwc3_gadget_wakeup(struct usb_gadget *g)
unsigned long flags;
int ret;

+ if (!dwc->wakeup_configured) {
+ dev_err(dwc->dev, "remote wakeup not configured\n");
+ return -EINVAL;
+ }
+
spin_lock_irqsave(&dwc->lock, flags);
- ret = __dwc3_gadget_wakeup(dwc);
+ if (!dwc->gadget->wakeup_armed) {
+ dev_err(dwc->dev, "not armed for remote wakeup\n");
+ spin_unlock_irqrestore(&dwc->lock, flags);
+ return -EINVAL;
+ }
+ ret = __dwc3_gadget_wakeup(dwc, true);
+
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);
+ unsigned long flags;
+
+ spin_lock_irqsave(&dwc->lock, flags);
+ dwc->wakeup_configured = !!set;
+ spin_unlock_irqrestore(&dwc->lock, flags);
+
+ return 0;
+}
+
static int dwc3_gadget_set_selfpowered(struct usb_gadget *g,
int is_selfpowered)
{
@@ -2978,6 +3028,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,
+ .set_remote_wakeup = dwc3_gadget_set_remote_wakeup,
.set_selfpowered = dwc3_gadget_set_selfpowered,
.pullup = dwc3_gadget_pullup,
.udc_start = dwc3_gadget_start,
@@ -3819,6 +3870,8 @@ static void dwc3_gadget_disconnect_interrupt(struct dwc3 *dwc)

dwc->gadget->speed = USB_SPEED_UNKNOWN;
dwc->setup_packet_pending = false;
+ dwc->gadget->wakeup_armed = false;
+ dwc3_gadget_enable_linksts_evts(dwc, false);
usb_gadget_set_state(dwc->gadget, USB_STATE_NOTATTACHED);

if (dwc->ep0state != EP0_SETUP_PHASE) {
@@ -3912,6 +3965,8 @@ static void dwc3_gadget_reset_interrupt(struct dwc3 *dwc)
reg &= ~DWC3_DCTL_TSTCTRL_MASK;
dwc3_gadget_dctl_write_safe(dwc, reg);
dwc->test_mode = false;
+ dwc->gadget->wakeup_armed = false;
+ dwc3_gadget_enable_linksts_evts(dwc, false);
dwc3_clear_stall_all_ep(dwc);

/* Reset device address to zero */
@@ -4064,7 +4119,7 @@ static void dwc3_gadget_conndone_interrupt(struct dwc3 *dwc)
*/
}

-static void dwc3_gadget_wakeup_interrupt(struct dwc3 *dwc)
+static void dwc3_gadget_wakeup_interrupt(struct dwc3 *dwc, unsigned int evtinfo)
{
/*
* TODO take core out of low power mode when that's
@@ -4076,6 +4131,8 @@ static void dwc3_gadget_wakeup_interrupt(struct dwc3 *dwc)
dwc->gadget_driver->resume(dwc->gadget);
spin_lock(&dwc->lock);
}
+
+ dwc->link_state = evtinfo & DWC3_LINK_STATE_MASK;
}

static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc,
@@ -4157,6 +4214,10 @@ static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc,
}

switch (next) {
+ case DWC3_LINK_STATE_U0:
+ dwc3_gadget_enable_linksts_evts(dwc, false);
+ dwc3_resume_gadget(dwc);
+ break;
case DWC3_LINK_STATE_U1:
if (dwc->speed == USB_SPEED_SUPER)
dwc3_suspend_gadget(dwc);
@@ -4225,7 +4286,7 @@ static void dwc3_gadget_interrupt(struct dwc3 *dwc,
dwc3_gadget_conndone_interrupt(dwc);
break;
case DWC3_DEVICE_EVENT_WAKEUP:
- dwc3_gadget_wakeup_interrupt(dwc);
+ dwc3_gadget_wakeup_interrupt(dwc, event->event_info);
break;
case DWC3_DEVICE_EVENT_HIBER_REQ:
if (dev_WARN_ONCE(dwc->dev, !dwc->has_hibernation,
@@ -4485,6 +4546,7 @@ int dwc3_gadget_init(struct dwc3 *dwc)
dwc->gadget->sg_supported = true;
dwc->gadget->name = "dwc3-gadget";
dwc->gadget->lpm_capable = !dwc->usb2_gadget_lpm_disable;
+ dwc->gadget->wakeup_capable = true;

/*
* FIXME We might be setting max_speed to <SUPER, however versions
--
2.7.4


2023-02-13 19:31:47

by Elson Serrao

[permalink] [raw]
Subject: [PATCH v4 5/5] usb: gadget: f_ecm: Add suspend/resume and remote wakeup support

When host sends a suspend notification to the device, handle
the suspend callbacks in the function driver. Enhanced super
speed devices can support function suspend feature to put the
function in suspend state. Handle function suspend callback.

Depending on the remote wakeup capability the device can either
trigger a remote wakeup or wait for the host initiated resume to
start data transfer again.

Signed-off-by: Elson Roy Serrao <[email protected]>
---
drivers/usb/gadget/function/f_ecm.c | 68 +++++++++++++++++++++++++++++++++++
drivers/usb/gadget/function/u_ether.c | 63 ++++++++++++++++++++++++++++++++
drivers/usb/gadget/function/u_ether.h | 4 +++
3 files changed, 135 insertions(+)

diff --git a/drivers/usb/gadget/function/f_ecm.c b/drivers/usb/gadget/function/f_ecm.c
index a7ab30e..d50c1a4 100644
--- a/drivers/usb/gadget/function/f_ecm.c
+++ b/drivers/usb/gadget/function/f_ecm.c
@@ -633,6 +633,8 @@ static void ecm_disable(struct usb_function *f)

usb_ep_disable(ecm->notify);
ecm->notify->desc = NULL;
+ f->func_suspended = false;
+ f->func_wakeup_armed = false;
}

/*-------------------------------------------------------------------------*/
@@ -885,6 +887,68 @@ static struct usb_function_instance *ecm_alloc_inst(void)
return &opts->func_inst;
}

+static void ecm_suspend(struct usb_function *f)
+{
+ struct f_ecm *ecm = func_to_ecm(f);
+ struct usb_composite_dev *cdev = ecm->port.func.config->cdev;
+
+ if (f->func_suspended) {
+ DBG(cdev, "Function already suspended\n");
+ return;
+ }
+
+ DBG(cdev, "ECM Suspend\n");
+
+ gether_suspend(&ecm->port);
+}
+
+static void ecm_resume(struct usb_function *f)
+{
+ struct f_ecm *ecm = func_to_ecm(f);
+ struct usb_composite_dev *cdev = ecm->port.func.config->cdev;
+
+ /*
+ * If the function is in USB3 Function Suspend state, resume is
+ * canceled. In this case resume is done by a Function Resume request.
+ */
+ if (f->func_suspended)
+ return;
+
+ DBG(cdev, "ECM Resume\n");
+
+ gether_resume(&ecm->port);
+}
+
+static int ecm_get_status(struct usb_function *f)
+{
+ return (f->func_wakeup_armed ? USB_INTRF_STAT_FUNC_RW : 0) |
+ USB_INTRF_STAT_FUNC_RW_CAP;
+}
+
+static int ecm_func_suspend(struct usb_function *f, u8 options)
+{
+ struct f_ecm *ecm = func_to_ecm(f);
+ struct usb_composite_dev *cdev = ecm->port.func.config->cdev;
+
+ DBG(cdev, "func susp %u cmd\n", options);
+
+ f->func_wakeup_armed = !!(options & (USB_INTRF_FUNC_SUSPEND_RW >> 8));
+
+ if (options & (USB_INTRF_FUNC_SUSPEND_LP >> 8)) {
+ if (!f->func_suspended) {
+ ecm_suspend(f);
+ f->func_suspended = true;
+ }
+ } else {
+ if (f->func_suspended) {
+ f->func_suspended = false;
+ ecm_resume(f);
+ }
+ }
+
+ return 0;
+}
+
static void ecm_free(struct usb_function *f)
{
struct f_ecm *ecm;
@@ -952,6 +1016,10 @@ static struct usb_function *ecm_alloc(struct usb_function_instance *fi)
ecm->port.func.setup = ecm_setup;
ecm->port.func.disable = ecm_disable;
ecm->port.func.free_func = ecm_free;
+ ecm->port.func.suspend = ecm_suspend;
+ ecm->port.func.get_status = ecm_get_status;
+ ecm->port.func.func_suspend = ecm_func_suspend;
+ ecm->port.func.resume = ecm_resume;

return &ecm->port.func;
}
diff --git a/drivers/usb/gadget/function/u_ether.c b/drivers/usb/gadget/function/u_ether.c
index 8f12f3f..4e54089 100644
--- a/drivers/usb/gadget/function/u_ether.c
+++ b/drivers/usb/gadget/function/u_ether.c
@@ -471,6 +471,20 @@ static inline int is_promisc(u16 cdc_filter)
return cdc_filter & USB_CDC_PACKET_TYPE_PROMISCUOUS;
}

+static int ether_wakeup_host(struct gether *port)
+{
+ int ret;
+ struct usb_function *func = &port->func;
+ struct usb_gadget *gadget = func->config->cdev->gadget;
+
+ if (func->func_suspended)
+ ret = usb_func_wakeup(func);
+ else
+ ret = usb_gadget_wakeup(gadget);
+
+ return ret;
+}
+
static netdev_tx_t eth_start_xmit(struct sk_buff *skb,
struct net_device *net)
{
@@ -490,6 +504,15 @@ static netdev_tx_t eth_start_xmit(struct sk_buff *skb,
in = NULL;
cdc_filter = 0;
}
+
+ if (dev->port_usb->is_suspend) {
+ DBG(dev, "Port suspended. Triggering wakeup\n");
+ netif_stop_queue(net);
+ spin_unlock_irqrestore(&dev->lock, flags);
+ ether_wakeup_host(dev->port_usb);
+ return NETDEV_TX_BUSY;
+ }
+
spin_unlock_irqrestore(&dev->lock, flags);

if (!in) {
@@ -1046,6 +1069,45 @@ int gether_set_ifname(struct net_device *net, const char *name, int len)
}
EXPORT_SYMBOL_GPL(gether_set_ifname);

+void gether_suspend(struct gether *link)
+{
+ struct eth_dev *dev = link->ioport;
+ unsigned long flags;
+
+ if (!dev)
+ return;
+
+ if (atomic_read(&dev->tx_qlen)) {
+ /*
+ * There is a transfer in progress. So we trigger a remote
+ * wakeup to inform the host.
+ */
+ ether_wakeup_host(dev->port_usb);
+ return;
+ }
+ spin_lock_irqsave(&dev->lock, flags);
+ link->is_suspend = true;
+ spin_unlock_irqrestore(&dev->lock, flags);
+}
+EXPORT_SYMBOL_GPL(gether_suspend);
+
+void gether_resume(struct gether *link)
+{
+ struct eth_dev *dev = link->ioport;
+ unsigned long flags;
+
+ if (!dev)
+ return;
+
+ if (netif_queue_stopped(dev->net))
+ netif_start_queue(dev->net);
+
+ spin_lock_irqsave(&dev->lock, flags);
+ link->is_suspend = false;
+ spin_unlock_irqrestore(&dev->lock, flags);
+}
+EXPORT_SYMBOL_GPL(gether_resume);
+
/*
* gether_cleanup - remove Ethernet-over-USB device
* Context: may sleep
@@ -1208,6 +1270,7 @@ void gether_disconnect(struct gether *link)

spin_lock(&dev->lock);
dev->port_usb = NULL;
+ link->is_suspend = false;
spin_unlock(&dev->lock);
}
EXPORT_SYMBOL_GPL(gether_disconnect);
diff --git a/drivers/usb/gadget/function/u_ether.h b/drivers/usb/gadget/function/u_ether.h
index 4014454..851ee10 100644
--- a/drivers/usb/gadget/function/u_ether.h
+++ b/drivers/usb/gadget/function/u_ether.h
@@ -79,6 +79,7 @@ struct gether {
/* called on network open/close */
void (*open)(struct gether *);
void (*close)(struct gether *);
+ bool is_suspend;
};

#define DEFAULT_FILTER (USB_CDC_PACKET_TYPE_BROADCAST \
@@ -258,6 +259,9 @@ int gether_set_ifname(struct net_device *net, const char *name, int len);

void gether_cleanup(struct eth_dev *dev);

+void gether_suspend(struct gether *link);
+void gether_resume(struct gether *link);
+
/* connect/disconnect is handled by individual functions */
struct net_device *gether_connect(struct gether *);
void gether_disconnect(struct gether *);
--
2.7.4


2023-02-15 01:11:55

by Thinh Nguyen

[permalink] [raw]
Subject: Re: [PATCH v4 2/5] usb: dwc3: Add remote wakeup handling

On Mon, Feb 13, 2023, Elson Roy Serrao wrote:
> An usb device can initate a remote wakeup and bring the link out of
> suspend as dictated by the DEVICE_REMOTE_WAKEUP feature selector.
> Add support to handle this packet and set the remote wakeup capability.
>
> Some hosts may take longer time to initiate the resume signaling after
> device triggers a remote wakeup. So add async support to the wakeup API
> by enabling link status change events.
>
> Signed-off-by: Elson Roy Serrao <[email protected]>
> ---
> drivers/usb/dwc3/core.h | 2 ++
> drivers/usb/dwc3/ep0.c | 4 +++
> drivers/usb/dwc3/gadget.c | 74 +++++++++++++++++++++++++++++++++++++++++++----
> 3 files changed, 74 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 582ebd9..cc78236 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -1110,6 +1110,7 @@ struct dwc3_scratchpad_array {
> * 3 - Reserved
> * @dis_metastability_quirk: set to disable metastability quirk.
> * @dis_split_quirk: set to disable split boundary.
> + * @wakeup_configured: set if the device is configured for remote wakeup.
> * @imod_interval: set the interrupt moderation interval in 250ns
> * increments or 0 to disable.
> * @max_cfg_eps: current max number of IN eps used across all USB configs.
> @@ -1327,6 +1328,7 @@ struct dwc3 {
>
> unsigned dis_split_quirk:1;
> unsigned async_callbacks:1;
> + unsigned wakeup_configured:1;
>
> u16 imod_interval;
>
> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
> index 61de693..f90fa55 100644
> --- a/drivers/usb/dwc3/ep0.c
> +++ b/drivers/usb/dwc3/ep0.c
> @@ -356,6 +356,9 @@ static int dwc3_ep0_handle_status(struct dwc3 *dwc,
> usb_status |= 1 << USB_DEV_STAT_U1_ENABLED;
> if (reg & DWC3_DCTL_INITU2ENA)
> usb_status |= 1 << USB_DEV_STAT_U2_ENABLED;
> + } else {
> + usb_status |= dwc->gadget->wakeup_armed <<
> + USB_DEVICE_REMOTE_WAKEUP;
> }
>
> break;
> @@ -476,6 +479,7 @@ static int dwc3_ep0_handle_device(struct dwc3 *dwc,
>
> switch (wValue) {
> case USB_DEVICE_REMOTE_WAKEUP:
> + dwc->gadget->wakeup_armed = set;
> break;
> /*
> * 9.4.1 says only for SS, in AddressState only for
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 3c63fa9..b7fef5d 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -258,7 +258,7 @@ int dwc3_send_gadget_generic_command(struct dwc3 *dwc, unsigned int cmd,
> return ret;
> }
>
> -static int __dwc3_gadget_wakeup(struct dwc3 *dwc);
> +static int __dwc3_gadget_wakeup(struct dwc3 *dwc, bool async);
>
> /**
> * dwc3_send_gadget_ep_cmd - issue an endpoint command
> @@ -325,7 +325,7 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned int cmd,
>
> fallthrough;
> case DWC3_LINK_STATE_U3:
> - ret = __dwc3_gadget_wakeup(dwc);
> + ret = __dwc3_gadget_wakeup(dwc, false);
> dev_WARN_ONCE(dwc->dev, ret, "wakeup failed --> %d\n",
> ret);
> break;
> @@ -2269,6 +2269,22 @@ static const struct usb_ep_ops dwc3_gadget_ep_ops = {
>
> /* -------------------------------------------------------------------------- */
>
> +static void dwc3_gadget_enable_linksts_evts(struct dwc3 *dwc, bool set)
> +{
> + u32 reg;
> +
> + if (DWC3_VER_IS_PRIOR(DWC3, 250A))
> + return;
> +
> + reg = dwc3_readl(dwc->regs, DWC3_DEVTEN);
> + if (set)
> + reg |= DWC3_DEVTEN_ULSTCNGEN;
> + else
> + reg &= ~DWC3_DEVTEN_ULSTCNGEN;
> +
> + dwc3_writel(dwc->regs, DWC3_DEVTEN, reg);
> +}
> +
> static int dwc3_gadget_get_frame(struct usb_gadget *g)
> {
> struct dwc3 *dwc = gadget_to_dwc(g);
> @@ -2276,7 +2292,7 @@ static int dwc3_gadget_get_frame(struct usb_gadget *g)
> return __dwc3_gadget_get_frame(dwc);
> }
>
> -static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
> +static int __dwc3_gadget_wakeup(struct dwc3 *dwc, bool async)
> {
> int retries;
>
> @@ -2307,9 +2323,13 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
> return -EINVAL;
> }
>
> + if (async)
> + dwc3_gadget_enable_linksts_evts(dwc, true);
> +
> ret = dwc3_gadget_set_link_state(dwc, DWC3_LINK_STATE_RECOV);
> if (ret < 0) {
> dev_err(dwc->dev, "failed to put link in Recovery\n");
> + dwc3_gadget_enable_linksts_evts(dwc, false);
> return ret;
> }
>
> @@ -2321,6 +2341,13 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
> dwc3_writel(dwc->regs, DWC3_DCTL, reg);
> }
>
> + /*
> + * Since link status change events are enabled we will receive
> + * an U0 event when wakeup is successful. So bail out.
> + */
> + if (async)
> + return 0;
> +
> /* poll until Link State changes to ON */
> retries = 20000;
>
> @@ -2346,13 +2373,36 @@ static int dwc3_gadget_wakeup(struct usb_gadget *g)
> unsigned long flags;
> int ret;
>
> + if (!dwc->wakeup_configured) {
> + dev_err(dwc->dev, "remote wakeup not configured\n");
> + return -EINVAL;
> + }
> +
> spin_lock_irqsave(&dwc->lock, flags);
> - ret = __dwc3_gadget_wakeup(dwc);
> + if (!dwc->gadget->wakeup_armed) {
> + dev_err(dwc->dev, "not armed for remote wakeup\n");
> + spin_unlock_irqrestore(&dwc->lock, flags);
> + return -EINVAL;
> + }
> + ret = __dwc3_gadget_wakeup(dwc, true);
> +
> 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);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&dwc->lock, flags);
> + dwc->wakeup_configured = !!set;
> + spin_unlock_irqrestore(&dwc->lock, flags);
> +
> + return 0;
> +}
> +
> static int dwc3_gadget_set_selfpowered(struct usb_gadget *g,
> int is_selfpowered)
> {
> @@ -2978,6 +3028,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,
> + .set_remote_wakeup = dwc3_gadget_set_remote_wakeup,
> .set_selfpowered = dwc3_gadget_set_selfpowered,
> .pullup = dwc3_gadget_pullup,
> .udc_start = dwc3_gadget_start,
> @@ -3819,6 +3870,8 @@ static void dwc3_gadget_disconnect_interrupt(struct dwc3 *dwc)
>
> dwc->gadget->speed = USB_SPEED_UNKNOWN;
> dwc->setup_packet_pending = false;
> + dwc->gadget->wakeup_armed = false;
> + dwc3_gadget_enable_linksts_evts(dwc, false);
> usb_gadget_set_state(dwc->gadget, USB_STATE_NOTATTACHED);
>
> if (dwc->ep0state != EP0_SETUP_PHASE) {
> @@ -3912,6 +3965,8 @@ static void dwc3_gadget_reset_interrupt(struct dwc3 *dwc)
> reg &= ~DWC3_DCTL_TSTCTRL_MASK;
> dwc3_gadget_dctl_write_safe(dwc, reg);
> dwc->test_mode = false;
> + dwc->gadget->wakeup_armed = false;
> + dwc3_gadget_enable_linksts_evts(dwc, false);
> dwc3_clear_stall_all_ep(dwc);
>
> /* Reset device address to zero */
> @@ -4064,7 +4119,7 @@ static void dwc3_gadget_conndone_interrupt(struct dwc3 *dwc)
> */
> }
>
> -static void dwc3_gadget_wakeup_interrupt(struct dwc3 *dwc)
> +static void dwc3_gadget_wakeup_interrupt(struct dwc3 *dwc, unsigned int evtinfo)
> {
> /*
> * TODO take core out of low power mode when that's
> @@ -4076,6 +4131,8 @@ static void dwc3_gadget_wakeup_interrupt(struct dwc3 *dwc)
> dwc->gadget_driver->resume(dwc->gadget);
> spin_lock(&dwc->lock);
> }
> +
> + dwc->link_state = evtinfo & DWC3_LINK_STATE_MASK;
> }
>
> static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc,
> @@ -4157,6 +4214,10 @@ static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc,
> }
>
> switch (next) {
> + case DWC3_LINK_STATE_U0:
> + dwc3_gadget_enable_linksts_evts(dwc, false);
> + dwc3_resume_gadget(dwc);

I don't think this should be a problem for dwc_usb3 2.50a and prior. But
for a device that enables link state change event all the time, it would
call this every time link transitions to U0. I'm not sure what the side
effect can be. Can we add a wakeup_armed check here to prevent possible
regression?

Thanks,
Thinh

> + break;
> case DWC3_LINK_STATE_U1:
> if (dwc->speed == USB_SPEED_SUPER)
> dwc3_suspend_gadget(dwc);
> @@ -4225,7 +4286,7 @@ static void dwc3_gadget_interrupt(struct dwc3 *dwc,
> dwc3_gadget_conndone_interrupt(dwc);
> break;
> case DWC3_DEVICE_EVENT_WAKEUP:
> - dwc3_gadget_wakeup_interrupt(dwc);
> + dwc3_gadget_wakeup_interrupt(dwc, event->event_info);
> break;
> case DWC3_DEVICE_EVENT_HIBER_REQ:
> if (dev_WARN_ONCE(dwc->dev, !dwc->has_hibernation,
> @@ -4485,6 +4546,7 @@ int dwc3_gadget_init(struct dwc3 *dwc)
> dwc->gadget->sg_supported = true;
> dwc->gadget->name = "dwc3-gadget";
> dwc->gadget->lpm_capable = !dwc->usb2_gadget_lpm_disable;
> + dwc->gadget->wakeup_capable = true;
>
> /*
> * FIXME We might be setting max_speed to <SUPER, however versions
> --
> 2.7.4
>

2023-02-15 01:20:02

by Thinh Nguyen

[permalink] [raw]
Subject: Re: [PATCH v4 4/5] usb: dwc3: Add function suspend and function wakeup support

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

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
>

2023-02-15 02:09:28

by Elson Serrao

[permalink] [raw]
Subject: Re: [PATCH v4 2/5] usb: dwc3: Add remote wakeup handling



On 2/14/2023 5:09 PM, Thinh Nguyen wrote:
> On Mon, Feb 13, 2023, Elson Roy Serrao wrote:
>> An usb device can initate a remote wakeup and bring the link out of
>> suspend as dictated by the DEVICE_REMOTE_WAKEUP feature selector.
>> Add support to handle this packet and set the remote wakeup capability.
>>
>> Some hosts may take longer time to initiate the resume signaling after
>> device triggers a remote wakeup. So add async support to the wakeup API
>> by enabling link status change events.
>>
>> Signed-off-by: Elson Roy Serrao <[email protected]>
>> ---
>> drivers/usb/dwc3/core.h | 2 ++
>> drivers/usb/dwc3/ep0.c | 4 +++
>> drivers/usb/dwc3/gadget.c | 74 +++++++++++++++++++++++++++++++++++++++++++----
>> 3 files changed, 74 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>> index 582ebd9..cc78236 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -1110,6 +1110,7 @@ struct dwc3_scratchpad_array {
>> * 3 - Reserved
>> * @dis_metastability_quirk: set to disable metastability quirk.
>> * @dis_split_quirk: set to disable split boundary.
>> + * @wakeup_configured: set if the device is configured for remote wakeup.
>> * @imod_interval: set the interrupt moderation interval in 250ns
>> * increments or 0 to disable.
>> * @max_cfg_eps: current max number of IN eps used across all USB configs.
>> @@ -1327,6 +1328,7 @@ struct dwc3 {
>>
>> unsigned dis_split_quirk:1;
>> unsigned async_callbacks:1;
>> + unsigned wakeup_configured:1;
>>
>> u16 imod_interval;
>>
>> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
>> index 61de693..f90fa55 100644
>> --- a/drivers/usb/dwc3/ep0.c
>> +++ b/drivers/usb/dwc3/ep0.c
>> @@ -356,6 +356,9 @@ static int dwc3_ep0_handle_status(struct dwc3 *dwc,
>> usb_status |= 1 << USB_DEV_STAT_U1_ENABLED;
>> if (reg & DWC3_DCTL_INITU2ENA)
>> usb_status |= 1 << USB_DEV_STAT_U2_ENABLED;
>> + } else {
>> + usb_status |= dwc->gadget->wakeup_armed <<
>> + USB_DEVICE_REMOTE_WAKEUP;
>> }
>>
>> break;
>> @@ -476,6 +479,7 @@ static int dwc3_ep0_handle_device(struct dwc3 *dwc,
>>
>> switch (wValue) {
>> case USB_DEVICE_REMOTE_WAKEUP:
>> + dwc->gadget->wakeup_armed = set;
>> break;
>> /*
>> * 9.4.1 says only for SS, in AddressState only for
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 3c63fa9..b7fef5d 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -258,7 +258,7 @@ int dwc3_send_gadget_generic_command(struct dwc3 *dwc, unsigned int cmd,
>> return ret;
>> }
>>
>> -static int __dwc3_gadget_wakeup(struct dwc3 *dwc);
>> +static int __dwc3_gadget_wakeup(struct dwc3 *dwc, bool async);
>>
>> /**
>> * dwc3_send_gadget_ep_cmd - issue an endpoint command
>> @@ -325,7 +325,7 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned int cmd,
>>
>> fallthrough;
>> case DWC3_LINK_STATE_U3:
>> - ret = __dwc3_gadget_wakeup(dwc);
>> + ret = __dwc3_gadget_wakeup(dwc, false);
>> dev_WARN_ONCE(dwc->dev, ret, "wakeup failed --> %d\n",
>> ret);
>> break;
>> @@ -2269,6 +2269,22 @@ static const struct usb_ep_ops dwc3_gadget_ep_ops = {
>>
>> /* -------------------------------------------------------------------------- */
>>
>> +static void dwc3_gadget_enable_linksts_evts(struct dwc3 *dwc, bool set)
>> +{
>> + u32 reg;
>> +
>> + if (DWC3_VER_IS_PRIOR(DWC3, 250A))
>> + return;
>> +
>> + reg = dwc3_readl(dwc->regs, DWC3_DEVTEN);
>> + if (set)
>> + reg |= DWC3_DEVTEN_ULSTCNGEN;
>> + else
>> + reg &= ~DWC3_DEVTEN_ULSTCNGEN;
>> +
>> + dwc3_writel(dwc->regs, DWC3_DEVTEN, reg);
>> +}
>> +
>> static int dwc3_gadget_get_frame(struct usb_gadget *g)
>> {
>> struct dwc3 *dwc = gadget_to_dwc(g);
>> @@ -2276,7 +2292,7 @@ static int dwc3_gadget_get_frame(struct usb_gadget *g)
>> return __dwc3_gadget_get_frame(dwc);
>> }
>>
>> -static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
>> +static int __dwc3_gadget_wakeup(struct dwc3 *dwc, bool async)
>> {
>> int retries;
>>
>> @@ -2307,9 +2323,13 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
>> return -EINVAL;
>> }
>>
>> + if (async)
>> + dwc3_gadget_enable_linksts_evts(dwc, true);
>> +
>> ret = dwc3_gadget_set_link_state(dwc, DWC3_LINK_STATE_RECOV);
>> if (ret < 0) {
>> dev_err(dwc->dev, "failed to put link in Recovery\n");
>> + dwc3_gadget_enable_linksts_evts(dwc, false);
>> return ret;
>> }
>>
>> @@ -2321,6 +2341,13 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
>> dwc3_writel(dwc->regs, DWC3_DCTL, reg);
>> }
>>
>> + /*
>> + * Since link status change events are enabled we will receive
>> + * an U0 event when wakeup is successful. So bail out.
>> + */
>> + if (async)
>> + return 0;
>> +
>> /* poll until Link State changes to ON */
>> retries = 20000;
>>
>> @@ -2346,13 +2373,36 @@ static int dwc3_gadget_wakeup(struct usb_gadget *g)
>> unsigned long flags;
>> int ret;
>>
>> + if (!dwc->wakeup_configured) {
>> + dev_err(dwc->dev, "remote wakeup not configured\n");
>> + return -EINVAL;
>> + }
>> +
>> spin_lock_irqsave(&dwc->lock, flags);
>> - ret = __dwc3_gadget_wakeup(dwc);
>> + if (!dwc->gadget->wakeup_armed) {
>> + dev_err(dwc->dev, "not armed for remote wakeup\n");
>> + spin_unlock_irqrestore(&dwc->lock, flags);
>> + return -EINVAL;
>> + }
>> + ret = __dwc3_gadget_wakeup(dwc, true);
>> +
>> 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);
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&dwc->lock, flags);
>> + dwc->wakeup_configured = !!set;
>> + spin_unlock_irqrestore(&dwc->lock, flags);
>> +
>> + return 0;
>> +}
>> +
>> static int dwc3_gadget_set_selfpowered(struct usb_gadget *g,
>> int is_selfpowered)
>> {
>> @@ -2978,6 +3028,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,
>> + .set_remote_wakeup = dwc3_gadget_set_remote_wakeup,
>> .set_selfpowered = dwc3_gadget_set_selfpowered,
>> .pullup = dwc3_gadget_pullup,
>> .udc_start = dwc3_gadget_start,
>> @@ -3819,6 +3870,8 @@ static void dwc3_gadget_disconnect_interrupt(struct dwc3 *dwc)
>>
>> dwc->gadget->speed = USB_SPEED_UNKNOWN;
>> dwc->setup_packet_pending = false;
>> + dwc->gadget->wakeup_armed = false;
>> + dwc3_gadget_enable_linksts_evts(dwc, false);
>> usb_gadget_set_state(dwc->gadget, USB_STATE_NOTATTACHED);
>>
>> if (dwc->ep0state != EP0_SETUP_PHASE) {
>> @@ -3912,6 +3965,8 @@ static void dwc3_gadget_reset_interrupt(struct dwc3 *dwc)
>> reg &= ~DWC3_DCTL_TSTCTRL_MASK;
>> dwc3_gadget_dctl_write_safe(dwc, reg);
>> dwc->test_mode = false;
>> + dwc->gadget->wakeup_armed = false;
>> + dwc3_gadget_enable_linksts_evts(dwc, false);
>> dwc3_clear_stall_all_ep(dwc);
>>
>> /* Reset device address to zero */
>> @@ -4064,7 +4119,7 @@ static void dwc3_gadget_conndone_interrupt(struct dwc3 *dwc)
>> */
>> }
>>
>> -static void dwc3_gadget_wakeup_interrupt(struct dwc3 *dwc)
>> +static void dwc3_gadget_wakeup_interrupt(struct dwc3 *dwc, unsigned int evtinfo)
>> {
>> /*
>> * TODO take core out of low power mode when that's
>> @@ -4076,6 +4131,8 @@ static void dwc3_gadget_wakeup_interrupt(struct dwc3 *dwc)
>> dwc->gadget_driver->resume(dwc->gadget);
>> spin_lock(&dwc->lock);
>> }
>> +
>> + dwc->link_state = evtinfo & DWC3_LINK_STATE_MASK;
>> }
>>
>> static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc,
>> @@ -4157,6 +4214,10 @@ static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc,
>> }
>>
>> switch (next) {
>> + case DWC3_LINK_STATE_U0:
>> + dwc3_gadget_enable_linksts_evts(dwc, false);
>> + dwc3_resume_gadget(dwc);
>
> I don't think this should be a problem for dwc_usb3 2.50a and prior. But
> for a device that enables link state change event all the time, it would
> call this every time link transitions to U0. I'm not sure what the side
> effect can be. Can we add a wakeup_armed check here to prevent possible
> regression?
>
> Thanks,
> Thinh
>
Sure. I will upload v5 with this modification.

Thanks
Elson
>> + break;
>> case DWC3_LINK_STATE_U1:
>> if (dwc->speed == USB_SPEED_SUPER)
>> dwc3_suspend_gadget(dwc);
>> @@ -4225,7 +4286,7 @@ static void dwc3_gadget_interrupt(struct dwc3 *dwc,
>> dwc3_gadget_conndone_interrupt(dwc);
>> break;
>> case DWC3_DEVICE_EVENT_WAKEUP:
>> - dwc3_gadget_wakeup_interrupt(dwc);
>> + dwc3_gadget_wakeup_interrupt(dwc, event->event_info);
>> break;
>> case DWC3_DEVICE_EVENT_HIBER_REQ:
>> if (dev_WARN_ONCE(dwc->dev, !dwc->has_hibernation,
>> @@ -4485,6 +4546,7 @@ int dwc3_gadget_init(struct dwc3 *dwc)
>> dwc->gadget->sg_supported = true;
>> dwc->gadget->name = "dwc3-gadget";
>> dwc->gadget->lpm_capable = !dwc->usb2_gadget_lpm_disable;
>> + dwc->gadget->wakeup_capable = true;
>>
>> /*
>> * FIXME We might be setting max_speed to <SUPER, however versions
>> --
>> 2.7.4