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 | 110 +++++++++++++++++++++++++++++++---
drivers/usb/gadget/composite.c | 50 +++++++++++++++-
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 | 46 ++++++++++++++
drivers/usb/gadget/udc/trace.h | 5 ++
include/linux/usb/composite.h | 6 ++
include/linux/usb/gadget.h | 12 ++++
12 files changed, 371 insertions(+), 16 deletions(-)
--
2.7.4
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 | 73 ++++++++++++++++++++++++++++++++++++++++++-----
3 files changed, 72 insertions(+), 7 deletions(-)
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 8f9959b..ff6e6f6 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.
+ * @rw_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.
@@ -1326,6 +1327,7 @@ struct dwc3 {
unsigned dis_split_quirk:1;
unsigned async_callbacks:1;
+ unsigned rw_configured:1;
u16 imod_interval;
diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
index 61de693..cd7c0cb 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->rw_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->rw_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 89dcfac..d0b9917 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,19 @@ static const struct usb_ep_ops dwc3_gadget_ep_ops = {
/* -------------------------------------------------------------------------- */
+static void dwc3_gadget_enable_linksts_evts(struct dwc3 *dwc, bool set)
+{
+ u32 reg;
+
+ 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 +2289,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;
@@ -2296,9 +2309,14 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
link_state = DWC3_DSTS_USBLNKST(reg);
switch (link_state) {
+ case DWC3_LINK_STATE_U3: /* in HS, means SUSPEND */
+ if (!dwc->rw_configured) {
+ dev_err(dwc->dev,
+ "device not configured for remote wakeup\n");
+ return -EINVAL;
+ }
case DWC3_LINK_STATE_RESET:
case DWC3_LINK_STATE_RX_DET: /* in HS, means Early Suspend */
- case DWC3_LINK_STATE_U3: /* in HS, means SUSPEND */
case DWC3_LINK_STATE_U2: /* in HS, means Sleep (L1) */
case DWC3_LINK_STATE_U1:
case DWC3_LINK_STATE_RESUME:
@@ -2307,9 +2325,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 +2343,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;
@@ -2347,12 +2376,30 @@ static int dwc3_gadget_wakeup(struct usb_gadget *g)
int ret;
spin_lock_irqsave(&dwc->lock, flags);
- ret = __dwc3_gadget_wakeup(dwc);
+ if (!dwc->gadget->rw_armed) {
+ dev_err(dwc->dev, "%s:remote wakeup not enabled\n", __func__);
+ 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_remotewakeup(struct usb_gadget *g, int set)
+{
+ struct dwc3 *dwc = gadget_to_dwc(g);
+ unsigned long flags;
+
+ spin_lock_irqsave(&dwc->lock, flags);
+ dwc->rw_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 +3025,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_remotewakeup = dwc3_gadget_set_remotewakeup,
.set_selfpowered = dwc3_gadget_set_selfpowered,
.pullup = dwc3_gadget_pullup,
.udc_start = dwc3_gadget_start,
@@ -3821,6 +3869,8 @@ static void dwc3_gadget_disconnect_interrupt(struct dwc3 *dwc)
dwc->gadget->speed = USB_SPEED_UNKNOWN;
dwc->setup_packet_pending = false;
+ dwc->gadget->rw_armed = false;
+ dwc3_gadget_enable_linksts_evts(dwc, false);
usb_gadget_set_state(dwc->gadget, USB_STATE_NOTATTACHED);
if (dwc->ep0state != EP0_SETUP_PHASE) {
@@ -3914,6 +3964,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->rw_armed = false;
+ dwc3_gadget_enable_linksts_evts(dwc, false);
dwc3_clear_stall_all_ep(dwc);
/* Reset device address to zero */
@@ -4066,7 +4118,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
@@ -4078,6 +4130,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,
@@ -4159,6 +4213,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);
@@ -4227,7 +4285,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,
@@ -4487,6 +4545,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->rw_capable = dwc->gadget->ops->wakeup ? true : false;
/*
* FIXME We might be setting max_speed to <SUPER, however versions
--
2.7.4
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..c0f750a 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_rw_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_rw_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_rw_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
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 | 37 +++++++++++++++++++++++++++++++++++++
4 files changed, 46 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index ff6e6f6..d604bcc 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 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 cd7c0cb..b14187f 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 d0b9917..afb95af 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2388,6 +2388,42 @@ 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;
+
+ 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_remotewakeup(struct usb_gadget *g, int set)
{
struct dwc3 *dwc = gadget_to_dwc(g);
@@ -3025,6 +3061,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_remotewakeup = dwc3_gadget_set_remotewakeup,
.set_selfpowered = dwc3_gadget_set_selfpowered,
.pullup = dwc3_gadget_pullup,
--
2.7.4
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 <[email protected]>
---
drivers/usb/gadget/composite.c | 26 ++++++++++++++++++++++++++
drivers/usb/gadget/udc/core.c | 19 +++++++++++++++++++
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 e459fb0..aa243d8 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -492,6 +492,32 @@ 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_rw_armed) {
+ ERROR(func->config->cdev, "func remote wakeup not enabled\n");
+ return -EINVAL;
+ }
+
+ DBG(func->config->cdev, "%s function wakeup\n", func->name);
+
+ 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);
+ return -EINVAL;
+ }
+
+ ret = usb_gadget_func_wakeup(func->config->cdev->gadget, id);
+
+ return ret;
+}
+EXPORT_SYMBOL(usb_func_wakeup);
+
static u8 encode_bMaxPower(enum usb_device_speed speed,
struct usb_configuration *c)
{
diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index 5874d4f..63b5944 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -846,6 +846,25 @@ int usb_gadget_activate(struct usb_gadget *gadget)
}
EXPORT_SYMBOL_GPL(usb_gadget_activate);
+/**
+ * usb_gadget_func_wakeup - sends function wake notification to the host.
+ * If the link is in low power state, first brings the link to active state.
+ * @gadget: controller used to wake up the host
+ * @interface_id: interface on which function wake notification is sent.
+ *
+ * Returns zero on success, else negative error code if the hardware
+ * doesn't support such attempts. Drivers must return device descriptors that
+ * report their ability to support this, or hosts won't enable it.
+ */
+int usb_gadget_func_wakeup(struct usb_gadget *gadget, int intf_id)
+{
+ if (!gadget->ops->func_wakeup)
+ return -EOPNOTSUPP;
+
+ return gadget->ops->func_wakeup(gadget, intf_id);
+}
+EXPORT_SYMBOL_GPL(usb_gadget_func_wakeup);
+
/* ------------------------------------------------------------------------- */
#ifdef CONFIG_HAS_DMA
diff --git a/include/linux/usb/composite.h b/include/linux/usb/composite.h
index 91d22c3..c9573ba 100644
--- a/include/linux/usb/composite.h
+++ b/include/linux/usb/composite.h
@@ -150,6 +150,9 @@ struct usb_os_desc_table {
* GetStatus() request when the recipient is Interface.
* @func_suspend: callback to be called when
* SetFeature(FUNCTION_SUSPEND) is reseived
+ * @func_suspended: Indicates whether the function is in function suspend state.
+ * @func_rw_armed: Indicates whether the function is armed by the host for
+ * wakeup signaling.
*
* A single USB function uses one or more interfaces, and should in most
* cases support operation at both full and high speeds. Each function is
@@ -220,6 +223,8 @@ struct usb_function {
int (*get_status)(struct usb_function *);
int (*func_suspend)(struct usb_function *,
u8 suspend_opt);
+ bool func_suspended;
+ bool func_rw_armed;
/* private: */
/* internals */
struct list_head list;
@@ -241,6 +246,7 @@ int config_ep_by_speed_and_alt(struct usb_gadget *g, struct usb_function *f,
int config_ep_by_speed(struct usb_gadget *g, struct usb_function *f,
struct usb_ep *_ep);
+int usb_func_wakeup(struct usb_function *func);
#define MAX_CONFIG_INTERFACES 16 /* arbitrary; max 255 */
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index 05d1449..1e73943 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -309,6 +309,7 @@ struct usb_udc;
struct usb_gadget_ops {
int (*get_frame)(struct usb_gadget *);
int (*wakeup)(struct usb_gadget *);
+ int (*func_wakeup)(struct usb_gadget *gadget, int intf_id);
int (*set_remotewakeup)(struct usb_gadget *, int set);
int (*set_selfpowered) (struct usb_gadget *, int is_selfpowered);
int (*vbus_session) (struct usb_gadget *, int is_active);
@@ -616,6 +617,7 @@ int usb_gadget_disconnect(struct usb_gadget *gadget);
int usb_gadget_deactivate(struct usb_gadget *gadget);
int usb_gadget_activate(struct usb_gadget *gadget);
int usb_gadget_check_config(struct usb_gadget *gadget);
+int usb_gadget_func_wakeup(struct usb_gadget *gadget, int intf_id);
#else
static inline int usb_gadget_frame_number(struct usb_gadget *gadget)
{ return 0; }
@@ -643,6 +645,8 @@ static inline int usb_gadget_activate(struct usb_gadget *gadget)
{ return 0; }
static inline int usb_gadget_check_config(struct usb_gadget *gadget)
{ return 0; }
+static inline int usb_gadget_func_wakeup(struct usb_gadget *gadget, int intf_id)
+{ return 0; }
#endif /* CONFIG_USB_GADGET */
/*-------------------------------------------------------------------------*/
--
2.7.4
The wakeup bit in the bmAttributes field indicates whether the device
is configured for remote wakeup. But this field should be allowed to
set only if the UDC supports such wakeup mechanism. So configure this
field based on UDC capability. Also inform the UDC whether the device
is configured for remote wakeup by implementing a gadget op.
Signed-off-by: Elson Roy Serrao <[email protected]>
---
drivers/usb/gadget/composite.c | 24 +++++++++++++++++++++++-
drivers/usb/gadget/udc/core.c | 27 +++++++++++++++++++++++++++
drivers/usb/gadget/udc/trace.h | 5 +++++
include/linux/usb/gadget.h | 8 ++++++++
4 files changed, 63 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index fa7dd6c..e459fb0 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -513,6 +513,19 @@ static u8 encode_bMaxPower(enum usb_device_speed speed,
return min(val, 900U) / 8;
}
+static void check_remote_wakeup_config(struct usb_gadget *gadget,
+ struct usb_configuration *c)
+{
+ if (USB_CONFIG_ATT_WAKEUP & c->bmAttributes) {
+ /* Reset the rw bit if gadget is not capable of it */
+ if (!gadget->rw_capable) {
+ INFO(c->cdev, "Clearing rw bit for config c.%d\n",
+ c->bConfigurationValue);
+ c->bmAttributes &= ~USB_CONFIG_ATT_WAKEUP;
+ }
+ }
+}
+
static int config_buf(struct usb_configuration *config,
enum usb_device_speed speed, void *buf, u8 type)
{
@@ -620,8 +633,12 @@ static int config_desc(struct usb_composite_dev *cdev, unsigned w_value)
continue;
}
- if (w_value == 0)
+ if (w_value == 0) {
+ /* Correctly configure the bmAttributes wakeup bit */
+ check_remote_wakeup_config(gadget, c);
+
return config_buf(c, speed, cdev->req->buf, type);
+ }
w_value--;
}
return -EINVAL;
@@ -1000,6 +1017,11 @@ static int set_config(struct usb_composite_dev *cdev,
else
usb_gadget_clear_selfpowered(gadget);
+ if (USB_CONFIG_ATT_WAKEUP & c->bmAttributes)
+ usb_gadget_set_remotewakeup(gadget, 1);
+ else
+ usb_gadget_set_remotewakeup(gadget, 0);
+
usb_gadget_vbus_draw(gadget, power);
if (result >= 0 && cdev->delayed_status)
result = USB_GADGET_DELAYED_STATUS;
diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index 23b0629..5874d4f 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -514,6 +514,33 @@ int usb_gadget_wakeup(struct usb_gadget *gadget)
EXPORT_SYMBOL_GPL(usb_gadget_wakeup);
/**
+ * usb_gadget_set_remotewakeup - configures the device remote wakeup feature.
+ * @gadget:the device being configured for remote wakeup
+ * @set:value to be configured.
+ *
+ * set to one to enable remote wakeup feature and zero to disable it.
+ *
+ * returns zero on success, else negative errno.
+ */
+int usb_gadget_set_remotewakeup(struct usb_gadget *gadget, int set)
+{
+ int ret = 0;
+
+ if (!gadget->ops->set_remotewakeup) {
+ ret = -EOPNOTSUPP;
+ goto out;
+ }
+
+ ret = gadget->ops->set_remotewakeup(gadget, set);
+
+out:
+ trace_usb_gadget_set_remotewakeup(gadget, ret);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(usb_gadget_set_remotewakeup);
+
+/**
* usb_gadget_set_selfpowered - sets the device selfpowered feature.
* @gadget:the device being declared as self-powered
*
diff --git a/drivers/usb/gadget/udc/trace.h b/drivers/usb/gadget/udc/trace.h
index abdbcb1..a3314ce 100644
--- a/drivers/usb/gadget/udc/trace.h
+++ b/drivers/usb/gadget/udc/trace.h
@@ -91,6 +91,11 @@ DEFINE_EVENT(udc_log_gadget, usb_gadget_wakeup,
TP_ARGS(g, ret)
);
+DEFINE_EVENT(udc_log_gadget, usb_gadget_set_remotewakeup,
+ TP_PROTO(struct usb_gadget *g, int ret),
+ TP_ARGS(g, ret)
+);
+
DEFINE_EVENT(udc_log_gadget, usb_gadget_set_selfpowered,
TP_PROTO(struct usb_gadget *g, int ret),
TP_ARGS(g, ret)
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index dc3092c..05d1449 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -309,6 +309,7 @@ struct usb_udc;
struct usb_gadget_ops {
int (*get_frame)(struct usb_gadget *);
int (*wakeup)(struct usb_gadget *);
+ int (*set_remotewakeup)(struct usb_gadget *, int set);
int (*set_selfpowered) (struct usb_gadget *, int is_selfpowered);
int (*vbus_session) (struct usb_gadget *, int is_active);
int (*vbus_draw) (struct usb_gadget *, unsigned mA);
@@ -383,6 +384,8 @@ struct usb_gadget_ops {
* @connected: True if gadget is connected.
* @lpm_capable: If the gadget max_speed is FULL or HIGH, this flag
* indicates that it supports LPM as per the LPM ECN & errata.
+ * @rw_capable: True if gadget is capable of sending remote wakeup.
+ * @rw_armed: True if gadget is armed by the host for remote wakeup.
* @irq: the interrupt number for device controller.
* @id_number: a unique ID number for ensuring that gadget names are distinct
*
@@ -444,6 +447,8 @@ struct usb_gadget {
unsigned deactivated:1;
unsigned connected:1;
unsigned lpm_capable:1;
+ unsigned rw_capable:1;
+ unsigned rw_armed:1;
int irq;
int id_number;
};
@@ -600,6 +605,7 @@ static inline int gadget_is_otg(struct usb_gadget *g)
#if IS_ENABLED(CONFIG_USB_GADGET)
int usb_gadget_frame_number(struct usb_gadget *gadget);
int usb_gadget_wakeup(struct usb_gadget *gadget);
+int usb_gadget_set_remotewakeup(struct usb_gadget *gadget, int set);
int usb_gadget_set_selfpowered(struct usb_gadget *gadget);
int usb_gadget_clear_selfpowered(struct usb_gadget *gadget);
int usb_gadget_vbus_connect(struct usb_gadget *gadget);
@@ -615,6 +621,8 @@ static inline int usb_gadget_frame_number(struct usb_gadget *gadget)
{ return 0; }
static inline int usb_gadget_wakeup(struct usb_gadget *gadget)
{ return 0; }
+static inline int usb_gadget_set_remotewakeup(struct usb_gadget *gadget, int set)
+{ return 0; }
static inline int usb_gadget_set_selfpowered(struct usb_gadget *gadget)
{ return 0; }
static inline int usb_gadget_clear_selfpowered(struct usb_gadget *gadget)
--
2.7.4
On Mon, Feb 06, 2023 at 11:13:22AM -0800, Elson Roy Serrao wrote:
> The wakeup bit in the bmAttributes field indicates whether the device
> is configured for remote wakeup. But this field should be allowed to
> set only if the UDC supports such wakeup mechanism. So configure this
> field based on UDC capability. Also inform the UDC whether the device
> is configured for remote wakeup by implementing a gadget op.
>
> Signed-off-by: Elson Roy Serrao <[email protected]>
> ---
> drivers/usb/gadget/composite.c | 24 +++++++++++++++++++++++-
> drivers/usb/gadget/udc/core.c | 27 +++++++++++++++++++++++++++
> drivers/usb/gadget/udc/trace.h | 5 +++++
> include/linux/usb/gadget.h | 8 ++++++++
> 4 files changed, 63 insertions(+), 1 deletion(-)
> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> index dc3092c..05d1449 100644
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -309,6 +309,7 @@ struct usb_udc;
> struct usb_gadget_ops {
> int (*get_frame)(struct usb_gadget *);
> int (*wakeup)(struct usb_gadget *);
> + int (*set_remotewakeup)(struct usb_gadget *, int set);
> int (*set_selfpowered) (struct usb_gadget *, int is_selfpowered);
> int (*vbus_session) (struct usb_gadget *, int is_active);
> int (*vbus_draw) (struct usb_gadget *, unsigned mA);
> @@ -383,6 +384,8 @@ struct usb_gadget_ops {
> * @connected: True if gadget is connected.
> * @lpm_capable: If the gadget max_speed is FULL or HIGH, this flag
> * indicates that it supports LPM as per the LPM ECN & errata.
> + * @rw_capable: True if gadget is capable of sending remote wakeup.
> + * @rw_armed: True if gadget is armed by the host for remote wakeup.
Minor stylistic request: Could you choose something other than "rw" to
start these field names? For too many people, that abbreviation is
firmly associated with "read/write". Maybe just "wakeup"?
Alan Stern
On 2/6/2023 12:14 PM, Alan Stern wrote:
> On Mon, Feb 06, 2023 at 11:13:22AM -0800, Elson Roy Serrao wrote:
>> The wakeup bit in the bmAttributes field indicates whether the device
>> is configured for remote wakeup. But this field should be allowed to
>> set only if the UDC supports such wakeup mechanism. So configure this
>> field based on UDC capability. Also inform the UDC whether the device
>> is configured for remote wakeup by implementing a gadget op.
>>
>> Signed-off-by: Elson Roy Serrao <[email protected]>
>> ---
>> drivers/usb/gadget/composite.c | 24 +++++++++++++++++++++++-
>> drivers/usb/gadget/udc/core.c | 27 +++++++++++++++++++++++++++
>> drivers/usb/gadget/udc/trace.h | 5 +++++
>> include/linux/usb/gadget.h | 8 ++++++++
>> 4 files changed, 63 insertions(+), 1 deletion(-)
>
>> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
>> index dc3092c..05d1449 100644
>> --- a/include/linux/usb/gadget.h
>> +++ b/include/linux/usb/gadget.h
>> @@ -309,6 +309,7 @@ struct usb_udc;
>> struct usb_gadget_ops {
>> int (*get_frame)(struct usb_gadget *);
>> int (*wakeup)(struct usb_gadget *);
>> + int (*set_remotewakeup)(struct usb_gadget *, int set);
>> int (*set_selfpowered) (struct usb_gadget *, int is_selfpowered);
>> int (*vbus_session) (struct usb_gadget *, int is_active);
>> int (*vbus_draw) (struct usb_gadget *, unsigned mA);
>> @@ -383,6 +384,8 @@ struct usb_gadget_ops {
>> * @connected: True if gadget is connected.
>> * @lpm_capable: If the gadget max_speed is FULL or HIGH, this flag
>> * indicates that it supports LPM as per the LPM ECN & errata.
>> + * @rw_capable: True if gadget is capable of sending remote wakeup.
>> + * @rw_armed: True if gadget is armed by the host for remote wakeup.
>
> Minor stylistic request: Could you choose something other than "rw" to
> start these field names? For too many people, that abbreviation is
> firmly associated with "read/write". Maybe just "wakeup"?
>
> Alan Stern
Sure. Agree that "rw" is firmly associated with "read/write". Will just
rename it to "wakeup"
Thanks
Elson
Hi Elson,
On Mon, Feb 06, 2023, Elson Roy Serrao wrote:
> The wakeup bit in the bmAttributes field indicates whether the device
> is configured for remote wakeup. But this field should be allowed to
> set only if the UDC supports such wakeup mechanism. So configure this
> field based on UDC capability. Also inform the UDC whether the device
> is configured for remote wakeup by implementing a gadget op.
>
> Signed-off-by: Elson Roy Serrao <[email protected]>
> ---
> drivers/usb/gadget/composite.c | 24 +++++++++++++++++++++++-
> drivers/usb/gadget/udc/core.c | 27 +++++++++++++++++++++++++++
> drivers/usb/gadget/udc/trace.h | 5 +++++
> include/linux/usb/gadget.h | 8 ++++++++
> 4 files changed, 63 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> index fa7dd6c..e459fb0 100644
> --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -513,6 +513,19 @@ static u8 encode_bMaxPower(enum usb_device_speed speed,
> return min(val, 900U) / 8;
> }
>
> +static void check_remote_wakeup_config(struct usb_gadget *gadget,
> + struct usb_configuration *c)
> +{
> + if (USB_CONFIG_ATT_WAKEUP & c->bmAttributes) {
> + /* Reset the rw bit if gadget is not capable of it */
> + if (!gadget->rw_capable) {
Probably it's better to make sure we don't break the configuration for
other UDCs until this is implemented in their drivers. Let's add another
condition:
if (!gadget->rw_capable && gadget->ops->set_remotewakeup) {
...
}
> + INFO(c->cdev, "Clearing rw bit for config c.%d\n",
> + c->bConfigurationValue);
Perhaps a warning is better since we're overwriting the user's
configuration.
> + c->bmAttributes &= ~USB_CONFIG_ATT_WAKEUP;
> + }
> + }
> +}
> +
> static int config_buf(struct usb_configuration *config,
> enum usb_device_speed speed, void *buf, u8 type)
> {
> @@ -620,8 +633,12 @@ static int config_desc(struct usb_composite_dev *cdev, unsigned w_value)
> continue;
> }
>
> - if (w_value == 0)
> + if (w_value == 0) {
> + /* Correctly configure the bmAttributes wakeup bit */
> + check_remote_wakeup_config(gadget, c);
Checking here is too late. We should check in configfs_composite_bind().
> +
> return config_buf(c, speed, cdev->req->buf, type);
> + }
> w_value--;
> }
> return -EINVAL;
> @@ -1000,6 +1017,11 @@ static int set_config(struct usb_composite_dev *cdev,
> else
> usb_gadget_clear_selfpowered(gadget);
>
> + if (USB_CONFIG_ATT_WAKEUP & c->bmAttributes)
> + usb_gadget_set_remotewakeup(gadget, 1);
> + else
> + usb_gadget_set_remotewakeup(gadget, 0);
> +
> usb_gadget_vbus_draw(gadget, power);
> if (result >= 0 && cdev->delayed_status)
> result = USB_GADGET_DELAYED_STATUS;
> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> index 23b0629..5874d4f 100644
> --- a/drivers/usb/gadget/udc/core.c
> +++ b/drivers/usb/gadget/udc/core.c
> @@ -514,6 +514,33 @@ int usb_gadget_wakeup(struct usb_gadget *gadget)
> EXPORT_SYMBOL_GPL(usb_gadget_wakeup);
>
> /**
> + * usb_gadget_set_remotewakeup - configures the device remote wakeup feature.
> + * @gadget:the device being configured for remote wakeup
> + * @set:value to be configured.
> + *
> + * set to one to enable remote wakeup feature and zero to disable it.
> + *
> + * returns zero on success, else negative errno.
> + */
> +int usb_gadget_set_remotewakeup(struct usb_gadget *gadget, int set)
> +{
> + int ret = 0;
> +
> + if (!gadget->ops->set_remotewakeup) {
> + ret = -EOPNOTSUPP;
> + goto out;
> + }
> +
> + ret = gadget->ops->set_remotewakeup(gadget, set);
> +
> +out:
> + trace_usb_gadget_set_remotewakeup(gadget, ret);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(usb_gadget_set_remotewakeup);
> +
> +/**
> * usb_gadget_set_selfpowered - sets the device selfpowered feature.
> * @gadget:the device being declared as self-powered
> *
> diff --git a/drivers/usb/gadget/udc/trace.h b/drivers/usb/gadget/udc/trace.h
> index abdbcb1..a3314ce 100644
> --- a/drivers/usb/gadget/udc/trace.h
> +++ b/drivers/usb/gadget/udc/trace.h
> @@ -91,6 +91,11 @@ DEFINE_EVENT(udc_log_gadget, usb_gadget_wakeup,
> TP_ARGS(g, ret)
> );
>
> +DEFINE_EVENT(udc_log_gadget, usb_gadget_set_remotewakeup,
> + TP_PROTO(struct usb_gadget *g, int ret),
> + TP_ARGS(g, ret)
> +);
> +
> DEFINE_EVENT(udc_log_gadget, usb_gadget_set_selfpowered,
> TP_PROTO(struct usb_gadget *g, int ret),
> TP_ARGS(g, ret)
> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> index dc3092c..05d1449 100644
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -309,6 +309,7 @@ struct usb_udc;
> struct usb_gadget_ops {
> int (*get_frame)(struct usb_gadget *);
> int (*wakeup)(struct usb_gadget *);
> + int (*set_remotewakeup)(struct usb_gadget *, int set);
minor nit: can we change this name to set_remote_wakeup. It's a bit
easier to read IMO.
> int (*set_selfpowered) (struct usb_gadget *, int is_selfpowered);
> int (*vbus_session) (struct usb_gadget *, int is_active);
> int (*vbus_draw) (struct usb_gadget *, unsigned mA);
> @@ -383,6 +384,8 @@ struct usb_gadget_ops {
> * @connected: True if gadget is connected.
> * @lpm_capable: If the gadget max_speed is FULL or HIGH, this flag
> * indicates that it supports LPM as per the LPM ECN & errata.
> + * @rw_capable: True if gadget is capable of sending remote wakeup.
> + * @rw_armed: True if gadget is armed by the host for remote wakeup.
> * @irq: the interrupt number for device controller.
> * @id_number: a unique ID number for ensuring that gadget names are distinct
> *
> @@ -444,6 +447,8 @@ struct usb_gadget {
> unsigned deactivated:1;
> unsigned connected:1;
> unsigned lpm_capable:1;
> + unsigned rw_capable:1;
> + unsigned rw_armed:1;
> int irq;
> int id_number;
> };
> @@ -600,6 +605,7 @@ static inline int gadget_is_otg(struct usb_gadget *g)
> #if IS_ENABLED(CONFIG_USB_GADGET)
> int usb_gadget_frame_number(struct usb_gadget *gadget);
> int usb_gadget_wakeup(struct usb_gadget *gadget);
> +int usb_gadget_set_remotewakeup(struct usb_gadget *gadget, int set);
> int usb_gadget_set_selfpowered(struct usb_gadget *gadget);
> int usb_gadget_clear_selfpowered(struct usb_gadget *gadget);
> int usb_gadget_vbus_connect(struct usb_gadget *gadget);
> @@ -615,6 +621,8 @@ static inline int usb_gadget_frame_number(struct usb_gadget *gadget)
> { return 0; }
> static inline int usb_gadget_wakeup(struct usb_gadget *gadget)
> { return 0; }
> +static inline int usb_gadget_set_remotewakeup(struct usb_gadget *gadget, int set)
> +{ return 0; }
> static inline int usb_gadget_set_selfpowered(struct usb_gadget *gadget)
> { return 0; }
> static inline int usb_gadget_clear_selfpowered(struct usb_gadget *gadget)
> --
> 2.7.4
>
Thanks,
Thinh
On Mon, Feb 06, 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 | 73 ++++++++++++++++++++++++++++++++++++++++++-----
> 3 files changed, 72 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 8f9959b..ff6e6f6 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.
> + * @rw_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.
> @@ -1326,6 +1327,7 @@ struct dwc3 {
>
> unsigned dis_split_quirk:1;
> unsigned async_callbacks:1;
> + unsigned rw_configured:1;
>
> u16 imod_interval;
>
> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
> index 61de693..cd7c0cb 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->rw_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->rw_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 89dcfac..d0b9917 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,19 @@ static const struct usb_ep_ops dwc3_gadget_ep_ops = {
>
> /* -------------------------------------------------------------------------- */
>
> +static void dwc3_gadget_enable_linksts_evts(struct dwc3 *dwc, bool set)
> +{
> + u32 reg;
Add a check here to prevent disabling link state event if the controller
is dwc_usb3 2.50a. Some older controller always enables this event for a
quirk.
> +
> + 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 +2289,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;
>
> @@ -2296,9 +2309,14 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
> link_state = DWC3_DSTS_USBLNKST(reg);
>
> switch (link_state) {
> + case DWC3_LINK_STATE_U3: /* in HS, means SUSPEND */
It's also possible to do remote wakeup in L1 for highspeed.
> + if (!dwc->rw_configured) {
> + dev_err(dwc->dev,
> + "device not configured for remote wakeup\n");
> + return -EINVAL;
> + }
> case DWC3_LINK_STATE_RESET:
> case DWC3_LINK_STATE_RX_DET: /* in HS, means Early Suspend */
> - case DWC3_LINK_STATE_U3: /* in HS, means SUSPEND */
> case DWC3_LINK_STATE_U2: /* in HS, means Sleep (L1) */
> case DWC3_LINK_STATE_U1:
> case DWC3_LINK_STATE_RESUME:
> @@ -2307,9 +2325,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 +2343,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;
>
> @@ -2347,12 +2376,30 @@ static int dwc3_gadget_wakeup(struct usb_gadget *g)
> int ret;
>
> spin_lock_irqsave(&dwc->lock, flags);
> - ret = __dwc3_gadget_wakeup(dwc);
> + if (!dwc->gadget->rw_armed) {
> + dev_err(dwc->dev, "%s:remote wakeup not enabled\n", __func__);
> + 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_remotewakeup(struct usb_gadget *g, int set)
> +{
> + struct dwc3 *dwc = gadget_to_dwc(g);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&dwc->lock, flags);
> + dwc->rw_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 +3025,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_remotewakeup = dwc3_gadget_set_remotewakeup,
> .set_selfpowered = dwc3_gadget_set_selfpowered,
> .pullup = dwc3_gadget_pullup,
> .udc_start = dwc3_gadget_start,
> @@ -3821,6 +3869,8 @@ static void dwc3_gadget_disconnect_interrupt(struct dwc3 *dwc)
>
> dwc->gadget->speed = USB_SPEED_UNKNOWN;
> dwc->setup_packet_pending = false;
> + dwc->gadget->rw_armed = false;
> + dwc3_gadget_enable_linksts_evts(dwc, false);
> usb_gadget_set_state(dwc->gadget, USB_STATE_NOTATTACHED);
>
> if (dwc->ep0state != EP0_SETUP_PHASE) {
> @@ -3914,6 +3964,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->rw_armed = false;
> + dwc3_gadget_enable_linksts_evts(dwc, false);
> dwc3_clear_stall_all_ep(dwc);
>
> /* Reset device address to zero */
> @@ -4066,7 +4118,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
> @@ -4078,6 +4130,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,
> @@ -4159,6 +4213,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);
> @@ -4227,7 +4285,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,
> @@ -4487,6 +4545,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->rw_capable = dwc->gadget->ops->wakeup ? true : false;
Just set it to true here.
>
> /*
> * FIXME We might be setting max_speed to <SUPER, however versions
> --
> 2.7.4
>
Thanks,
Thinh
On Mon, Feb 06, 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 <[email protected]>
> ---
> drivers/usb/gadget/composite.c | 26 ++++++++++++++++++++++++++
> drivers/usb/gadget/udc/core.c | 19 +++++++++++++++++++
> 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 e459fb0..aa243d8 100644
> --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -492,6 +492,32 @@ 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_rw_armed) {
> + ERROR(func->config->cdev, "func remote wakeup not enabled\n");
> + return -EINVAL;
> + }
> +
> + DBG(func->config->cdev, "%s function wakeup\n", func->name);
Do we need this DBG print?
> +
> + 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);
> + return -EINVAL;
> + }
> +
> + ret = usb_gadget_func_wakeup(func->config->cdev->gadget, id);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(usb_func_wakeup);
> +
> static u8 encode_bMaxPower(enum usb_device_speed speed,
> struct usb_configuration *c)
> {
> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> index 5874d4f..63b5944 100644
> --- a/drivers/usb/gadget/udc/core.c
> +++ b/drivers/usb/gadget/udc/core.c
> @@ -846,6 +846,25 @@ int usb_gadget_activate(struct usb_gadget *gadget)
> }
> EXPORT_SYMBOL_GPL(usb_gadget_activate);
>
> +/**
> + * usb_gadget_func_wakeup - sends function wake notification to the host.
> + * If the link is in low power state, first brings the link to active state.
> + * @gadget: controller used to wake up the host
> + * @interface_id: interface on which function wake notification is sent.
> + *
> + * Returns zero on success, else negative error code if the hardware
On completion, the device notification is sent? So it's a synchonous
call right? Probably can't be called in interrupt context? Can you
document the expectation here.
Thanks,
Thinh
> + * doesn't support such attempts. Drivers must return device descriptors that
> + * report their ability to support this, or hosts won't enable it.
> + */
> +int usb_gadget_func_wakeup(struct usb_gadget *gadget, int intf_id)
> +{
> + if (!gadget->ops->func_wakeup)
> + return -EOPNOTSUPP;
> +
> + return gadget->ops->func_wakeup(gadget, intf_id);
> +}
> +EXPORT_SYMBOL_GPL(usb_gadget_func_wakeup);
> +
> /* ------------------------------------------------------------------------- */
>
> #ifdef CONFIG_HAS_DMA
> diff --git a/include/linux/usb/composite.h b/include/linux/usb/composite.h
> index 91d22c3..c9573ba 100644
> --- a/include/linux/usb/composite.h
> +++ b/include/linux/usb/composite.h
> @@ -150,6 +150,9 @@ struct usb_os_desc_table {
> * GetStatus() request when the recipient is Interface.
> * @func_suspend: callback to be called when
> * SetFeature(FUNCTION_SUSPEND) is reseived
> + * @func_suspended: Indicates whether the function is in function suspend state.
> + * @func_rw_armed: Indicates whether the function is armed by the host for
> + * wakeup signaling.
> *
> * A single USB function uses one or more interfaces, and should in most
> * cases support operation at both full and high speeds. Each function is
> @@ -220,6 +223,8 @@ struct usb_function {
> int (*get_status)(struct usb_function *);
> int (*func_suspend)(struct usb_function *,
> u8 suspend_opt);
> + bool func_suspended;
> + bool func_rw_armed;
> /* private: */
> /* internals */
> struct list_head list;
> @@ -241,6 +246,7 @@ int config_ep_by_speed_and_alt(struct usb_gadget *g, struct usb_function *f,
>
> int config_ep_by_speed(struct usb_gadget *g, struct usb_function *f,
> struct usb_ep *_ep);
> +int usb_func_wakeup(struct usb_function *func);
>
> #define MAX_CONFIG_INTERFACES 16 /* arbitrary; max 255 */
>
> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> index 05d1449..1e73943 100644
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -309,6 +309,7 @@ struct usb_udc;
> struct usb_gadget_ops {
> int (*get_frame)(struct usb_gadget *);
> int (*wakeup)(struct usb_gadget *);
> + int (*func_wakeup)(struct usb_gadget *gadget, int intf_id);
> int (*set_remotewakeup)(struct usb_gadget *, int set);
> int (*set_selfpowered) (struct usb_gadget *, int is_selfpowered);
> int (*vbus_session) (struct usb_gadget *, int is_active);
> @@ -616,6 +617,7 @@ int usb_gadget_disconnect(struct usb_gadget *gadget);
> int usb_gadget_deactivate(struct usb_gadget *gadget);
> int usb_gadget_activate(struct usb_gadget *gadget);
> int usb_gadget_check_config(struct usb_gadget *gadget);
> +int usb_gadget_func_wakeup(struct usb_gadget *gadget, int intf_id);
> #else
> static inline int usb_gadget_frame_number(struct usb_gadget *gadget)
> { return 0; }
> @@ -643,6 +645,8 @@ static inline int usb_gadget_activate(struct usb_gadget *gadget)
> { return 0; }
> static inline int usb_gadget_check_config(struct usb_gadget *gadget)
> { return 0; }
> +static inline int usb_gadget_func_wakeup(struct usb_gadget *gadget, int intf_id)
> +{ return 0; }
> #endif /* CONFIG_USB_GADGET */
>
> /*-------------------------------------------------------------------------*/
> --
> 2.7.4
>
Hi Elson,
On Mon, Feb 06, 2023, Elson Roy Serrao wrote:
> 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 | 110 +++++++++++++++++++++++++++++++---
> drivers/usb/gadget/composite.c | 50 +++++++++++++++-
> 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 | 46 ++++++++++++++
> drivers/usb/gadget/udc/trace.h | 5 ++
> include/linux/usb/composite.h | 6 ++
> include/linux/usb/gadget.h | 12 ++++
> 12 files changed, 371 insertions(+), 16 deletions(-)
>
> --
> 2.7.4
>
Hi Elson,
Thanks for your patches! I provided some comments. Hopefully they can be
merged soon.
Thanks,
Thinh
On 2/6/2023 5:24 PM, Thinh Nguyen wrote:
> Hi Elson,
>
> On Mon, Feb 06, 2023, Elson Roy Serrao wrote:
>> 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 | 110 +++++++++++++++++++++++++++++++---
>> drivers/usb/gadget/composite.c | 50 +++++++++++++++-
>> 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 | 46 ++++++++++++++
>> drivers/usb/gadget/udc/trace.h | 5 ++
>> include/linux/usb/composite.h | 6 ++
>> include/linux/usb/gadget.h | 12 ++++
>> 12 files changed, 371 insertions(+), 16 deletions(-)
>>
>> --
>> 2.7.4
>>
>
> Hi Elson,
>
> Thanks for your patches! I provided some comments. Hopefully they can be
> merged soon.
>
> Thanks,
> Thinh
Hi Thinh
Thank you for your feedback/comments. I will address them in v4.
Regards
Elson
On Mon, Feb 06, 2023 at 11:13:24AM -0800, 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 <[email protected]>
> ---
> drivers/usb/gadget/composite.c | 26 ++++++++++++++++++++++++++
> drivers/usb/gadget/udc/core.c | 19 +++++++++++++++++++
> 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 e459fb0..aa243d8 100644
> --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -492,6 +492,32 @@ 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_rw_armed) {
> + ERROR(func->config->cdev, "func remote wakeup not enabled\n");
> + return -EINVAL;
> + }
> +
> + DBG(func->config->cdev, "%s function wakeup\n", func->name);
> +
> + 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);
> + return -EINVAL;
> + }
> +
> + ret = usb_gadget_func_wakeup(func->config->cdev->gadget, id);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(usb_func_wakeup);
EXPORT_SYMBOL_GPL() please.
thanks,
greg k-h
Hi Elson,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on usb/usb-testing]
[also build test WARNING on usb/usb-next usb/usb-linus linus/master v6.2-rc7 next-20230207]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Elson-Roy-Serrao/usb-gadget-Properly-configure-the-device-for-remote-wakeup/20230207-031528
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
patch link: https://lore.kernel.org/r/1675710806-9735-3-git-send-email-quic_eserrao%40quicinc.com
patch subject: [PATCH v3 2/5] usb: dwc3: Add remote wakeup handling
config: i386-randconfig-a002-20230206 (https://download.01.org/0day-ci/archive/20230207/[email protected]/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/e0d9f3f5f168e36cdb599617634010326a1412af
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Elson-Roy-Serrao/usb-gadget-Properly-configure-the-device-for-remote-wakeup/20230207-031528
git checkout e0d9f3f5f168e36cdb599617634010326a1412af
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/usb/dwc3/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
>> drivers/usb/dwc3/gadget.c:2318:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
case DWC3_LINK_STATE_RESET:
^
drivers/usb/dwc3/gadget.c:2318:2: note: insert 'break;' to avoid fall-through
case DWC3_LINK_STATE_RESET:
^
break;
1 warning generated.
vim +2318 drivers/usb/dwc3/gadget.c
72246da40f3719af Felipe Balbi 2011-08-19 2291
e0d9f3f5f168e36c Elson Roy Serrao 2023-02-06 2292 static int __dwc3_gadget_wakeup(struct dwc3 *dwc, bool async)
72246da40f3719af Felipe Balbi 2011-08-19 2293 {
d6011f6fc21b4d4a Nicolas Saenz Julienne 2016-08-16 2294 int retries;
72246da40f3719af Felipe Balbi 2011-08-19 2295
218ef7b647e3367c Felipe Balbi 2016-04-04 2296 int ret;
72246da40f3719af Felipe Balbi 2011-08-19 2297 u32 reg;
72246da40f3719af Felipe Balbi 2011-08-19 2298
72246da40f3719af Felipe Balbi 2011-08-19 2299 u8 link_state;
72246da40f3719af Felipe Balbi 2011-08-19 2300
72246da40f3719af Felipe Balbi 2011-08-19 2301 /*
72246da40f3719af Felipe Balbi 2011-08-19 2302 * According to the Databook Remote wakeup request should
72246da40f3719af Felipe Balbi 2011-08-19 2303 * be issued only when the device is in early suspend state.
72246da40f3719af Felipe Balbi 2011-08-19 2304 *
72246da40f3719af Felipe Balbi 2011-08-19 2305 * We can check that via USB Link State bits in DSTS register.
72246da40f3719af Felipe Balbi 2011-08-19 2306 */
72246da40f3719af Felipe Balbi 2011-08-19 2307 reg = dwc3_readl(dwc->regs, DWC3_DSTS);
72246da40f3719af Felipe Balbi 2011-08-19 2308
72246da40f3719af Felipe Balbi 2011-08-19 2309 link_state = DWC3_DSTS_USBLNKST(reg);
72246da40f3719af Felipe Balbi 2011-08-19 2310
72246da40f3719af Felipe Balbi 2011-08-19 2311 switch (link_state) {
e0d9f3f5f168e36c Elson Roy Serrao 2023-02-06 2312 case DWC3_LINK_STATE_U3: /* in HS, means SUSPEND */
e0d9f3f5f168e36c Elson Roy Serrao 2023-02-06 2313 if (!dwc->rw_configured) {
e0d9f3f5f168e36c Elson Roy Serrao 2023-02-06 2314 dev_err(dwc->dev,
e0d9f3f5f168e36c Elson Roy Serrao 2023-02-06 2315 "device not configured for remote wakeup\n");
e0d9f3f5f168e36c Elson Roy Serrao 2023-02-06 2316 return -EINVAL;
e0d9f3f5f168e36c Elson Roy Serrao 2023-02-06 2317 }
d0550cd20e52558e Thinh Nguyen 2020-01-31 @2318 case DWC3_LINK_STATE_RESET:
72246da40f3719af Felipe Balbi 2011-08-19 2319 case DWC3_LINK_STATE_RX_DET: /* in HS, means Early Suspend */
c560e76319a94a3b Thinh Nguyen 2021-04-19 2320 case DWC3_LINK_STATE_U2: /* in HS, means Sleep (L1) */
c560e76319a94a3b Thinh Nguyen 2021-04-19 2321 case DWC3_LINK_STATE_U1:
d0550cd20e52558e Thinh Nguyen 2020-01-31 2322 case DWC3_LINK_STATE_RESUME:
72246da40f3719af Felipe Balbi 2011-08-19 2323 break;
72246da40f3719af Felipe Balbi 2011-08-19 2324 default:
218ef7b647e3367c Felipe Balbi 2016-04-04 2325 return -EINVAL;
72246da40f3719af Felipe Balbi 2011-08-19 2326 }
72246da40f3719af Felipe Balbi 2011-08-19 2327
e0d9f3f5f168e36c Elson Roy Serrao 2023-02-06 2328 if (async)
e0d9f3f5f168e36c Elson Roy Serrao 2023-02-06 2329 dwc3_gadget_enable_linksts_evts(dwc, true);
e0d9f3f5f168e36c Elson Roy Serrao 2023-02-06 2330
8598bde7fa125e85 Felipe Balbi 2012-01-02 2331 ret = dwc3_gadget_set_link_state(dwc, DWC3_LINK_STATE_RECOV);
8598bde7fa125e85 Felipe Balbi 2012-01-02 2332 if (ret < 0) {
8598bde7fa125e85 Felipe Balbi 2012-01-02 2333 dev_err(dwc->dev, "failed to put link in Recovery\n");
e0d9f3f5f168e36c Elson Roy Serrao 2023-02-06 2334 dwc3_gadget_enable_linksts_evts(dwc, false);
218ef7b647e3367c Felipe Balbi 2016-04-04 2335 return ret;
8598bde7fa125e85 Felipe Balbi 2012-01-02 2336 }
72246da40f3719af Felipe Balbi 2011-08-19 2337
802fde983e8a3391 Paul Zimmerman 2012-04-27 2338 /* Recent versions do this automatically */
9af21dd6faeba593 Thinh Nguyen 2020-04-11 2339 if (DWC3_VER_IS_PRIOR(DWC3, 194A)) {
72246da40f3719af Felipe Balbi 2011-08-19 2340 /* write zeroes to Link Change Request */
fcc023c726b5879d Felipe Balbi 2012-05-24 2341 reg = dwc3_readl(dwc->regs, DWC3_DCTL);
72246da40f3719af Felipe Balbi 2011-08-19 2342 reg &= ~DWC3_DCTL_ULSTCHNGREQ_MASK;
72246da40f3719af Felipe Balbi 2011-08-19 2343 dwc3_writel(dwc->regs, DWC3_DCTL, reg);
802fde983e8a3391 Paul Zimmerman 2012-04-27 2344 }
72246da40f3719af Felipe Balbi 2011-08-19 2345
e0d9f3f5f168e36c Elson Roy Serrao 2023-02-06 2346 /*
e0d9f3f5f168e36c Elson Roy Serrao 2023-02-06 2347 * Since link status change events are enabled we will receive
e0d9f3f5f168e36c Elson Roy Serrao 2023-02-06 2348 * an U0 event when wakeup is successful. So bail out.
e0d9f3f5f168e36c Elson Roy Serrao 2023-02-06 2349 */
e0d9f3f5f168e36c Elson Roy Serrao 2023-02-06 2350 if (async)
e0d9f3f5f168e36c Elson Roy Serrao 2023-02-06 2351 return 0;
e0d9f3f5f168e36c Elson Roy Serrao 2023-02-06 2352
1d046793958f128d Paul Zimmerman 2012-02-15 2353 /* poll until Link State changes to ON */
d6011f6fc21b4d4a Nicolas Saenz Julienne 2016-08-16 2354 retries = 20000;
72246da40f3719af Felipe Balbi 2011-08-19 2355
d6011f6fc21b4d4a Nicolas Saenz Julienne 2016-08-16 2356 while (retries--) {
72246da40f3719af Felipe Balbi 2011-08-19 2357 reg = dwc3_readl(dwc->regs, DWC3_DSTS);
72246da40f3719af Felipe Balbi 2011-08-19 2358
72246da40f3719af Felipe Balbi 2011-08-19 2359 /* in HS, means ON */
72246da40f3719af Felipe Balbi 2011-08-19 2360 if (DWC3_DSTS_USBLNKST(reg) == DWC3_LINK_STATE_U0)
72246da40f3719af Felipe Balbi 2011-08-19 2361 break;
72246da40f3719af Felipe Balbi 2011-08-19 2362 }
72246da40f3719af Felipe Balbi 2011-08-19 2363
72246da40f3719af Felipe Balbi 2011-08-19 2364 if (DWC3_DSTS_USBLNKST(reg) != DWC3_LINK_STATE_U0) {
72246da40f3719af Felipe Balbi 2011-08-19 2365 dev_err(dwc->dev, "failed to send remote wakeup\n");
218ef7b647e3367c Felipe Balbi 2016-04-04 2366 return -EINVAL;
72246da40f3719af Felipe Balbi 2011-08-19 2367 }
72246da40f3719af Felipe Balbi 2011-08-19 2368
218ef7b647e3367c Felipe Balbi 2016-04-04 2369 return 0;
218ef7b647e3367c Felipe Balbi 2016-04-04 2370 }
218ef7b647e3367c Felipe Balbi 2016-04-04 2371
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
On 2/6/2023 4:48 PM, Thinh Nguyen wrote:
> On Mon, Feb 06, 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 | 73 ++++++++++++++++++++++++++++++++++++++++++-----
>> 3 files changed, 72 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>> index 8f9959b..ff6e6f6 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.
>> + * @rw_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.
>> @@ -1326,6 +1327,7 @@ struct dwc3 {
>>
>> unsigned dis_split_quirk:1;
>> unsigned async_callbacks:1;
>> + unsigned rw_configured:1;
>>
>> u16 imod_interval;
>>
>> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
>> index 61de693..cd7c0cb 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->rw_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->rw_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 89dcfac..d0b9917 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,19 @@ static const struct usb_ep_ops dwc3_gadget_ep_ops = {
>>
>> /* -------------------------------------------------------------------------- */
>>
>> +static void dwc3_gadget_enable_linksts_evts(struct dwc3 *dwc, bool set)
>> +{
>> + u32 reg;
>
> Add a check here to prevent disabling link state event if the controller
> is dwc_usb3 2.50a. Some older controller always enables this event for a
> quirk.
>
>> +
>> + 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 +2289,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;
>>
>> @@ -2296,9 +2309,14 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
>> link_state = DWC3_DSTS_USBLNKST(reg);
>>
>> switch (link_state) {
>> + case DWC3_LINK_STATE_U3: /* in HS, means SUSPEND */
>
> It's also possible to do remote wakeup in L1 for highspeed.
>
The rw_configured flag here is in context of triggering remote wakeup
from bus suspend only.
The remote wakeup setting for l1 in HighSpeed is controlled through LPM
token and overrides/ignores the config desc bmAttributes wakeup bit.
Section 4.1 of USB2_LinkPowerMangement_ECN[final] spec
"The host system sets the Remote Wake Flag parameter in this request to
enable or disable the addressed device
for remote wake from L1. The value of this flag will temporarily (while
in L1) override the current setting of the
Remote Wake feature settable by the standard Set/ClearFeature() commands
defined in Universal Serial Bus Specification, revision 2.0, Chapter 9."
Please let me know if I am missing something.
Thanks
Elson
>> + if (!dwc->rw_configured) {
>> + dev_err(dwc->dev,
>> + "device not configured for remote wakeup\n");
>> + return -EINVAL;
>> + }
>> case DWC3_LINK_STATE_RESET:
>> case DWC3_LINK_STATE_RX_DET: /* in HS, means Early Suspend */
>> - case DWC3_LINK_STATE_U3: /* in HS, means SUSPEND */
>> case DWC3_LINK_STATE_U2: /* in HS, means Sleep (L1) */
>> case DWC3_LINK_STATE_U1:
>> case DWC3_LINK_STATE_RESUME:
>> @@ -2307,9 +2325,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 +2343,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;
>>
>> @@ -2347,12 +2376,30 @@ static int dwc3_gadget_wakeup(struct usb_gadget *g)
>> int ret;
>>
>> spin_lock_irqsave(&dwc->lock, flags);
>> - ret = __dwc3_gadget_wakeup(dwc);
>> + if (!dwc->gadget->rw_armed) {
>> + dev_err(dwc->dev, "%s:remote wakeup not enabled\n", __func__);
>> + 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_remotewakeup(struct usb_gadget *g, int set)
>> +{
>> + struct dwc3 *dwc = gadget_to_dwc(g);
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&dwc->lock, flags);
>> + dwc->rw_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 +3025,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_remotewakeup = dwc3_gadget_set_remotewakeup,
>> .set_selfpowered = dwc3_gadget_set_selfpowered,
>> .pullup = dwc3_gadget_pullup,
>> .udc_start = dwc3_gadget_start,
>> @@ -3821,6 +3869,8 @@ static void dwc3_gadget_disconnect_interrupt(struct dwc3 *dwc)
>>
>> dwc->gadget->speed = USB_SPEED_UNKNOWN;
>> dwc->setup_packet_pending = false;
>> + dwc->gadget->rw_armed = false;
>> + dwc3_gadget_enable_linksts_evts(dwc, false);
>> usb_gadget_set_state(dwc->gadget, USB_STATE_NOTATTACHED);
>>
>> if (dwc->ep0state != EP0_SETUP_PHASE) {
>> @@ -3914,6 +3964,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->rw_armed = false;
>> + dwc3_gadget_enable_linksts_evts(dwc, false);
>> dwc3_clear_stall_all_ep(dwc);
>>
>> /* Reset device address to zero */
>> @@ -4066,7 +4118,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
>> @@ -4078,6 +4130,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,
>> @@ -4159,6 +4213,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);
>> @@ -4227,7 +4285,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,
>> @@ -4487,6 +4545,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->rw_capable = dwc->gadget->ops->wakeup ? true : false;
>
> Just set it to true here.
>
>>
>> /*
>> * FIXME We might be setting max_speed to <SUPER, however versions
>> --
>> 2.7.4
>>
>
> Thanks,
> Thinh
On Tue, Feb 07, 2023, Elson Serrao wrote:
> On 2/6/2023 4:48 PM, Thinh Nguyen wrote:
> > > +static int __dwc3_gadget_wakeup(struct dwc3 *dwc, bool async)
> > > {
> > > int retries;
> > > @@ -2296,9 +2309,14 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
> > > link_state = DWC3_DSTS_USBLNKST(reg);
> > > switch (link_state) {
> > > + case DWC3_LINK_STATE_U3: /* in HS, means SUSPEND */
> >
> > It's also possible to do remote wakeup in L1 for highspeed.
> >
>
> The rw_configured flag here is in context of triggering remote wakeup from
> bus suspend only.
>
> The remote wakeup setting for l1 in HighSpeed is controlled through LPM
> token and overrides/ignores the config desc bmAttributes wakeup bit.
>
> Section 4.1 of USB2_LinkPowerMangement_ECN[final] spec "The host system sets the Remote Wake Flag parameter in this request to
> enable or disable the addressed device
> for remote wake from L1. The value of this flag will temporarily (while in
> L1) override the current setting of the
> Remote Wake feature settable by the standard Set/ClearFeature() commands
> defined in Universal Serial Bus Specification, revision 2.0, Chapter 9."
>
> Please let me know if I am missing something.
>
It overrides the setting of the SetFeature request, not the device
configuration.
The rw_configured reflects the user configuration. Whether the host
tries to enable the remote wakeup through SetFeature request or LPM
token, the device should operate within the user configuration
limitation.
If the configuration indicates that it doesn't support remote wakeup, we
should prevent unexpected behavior from the device. For simplicity, we
can just return failure to wakeup for all states.
Thanks,
Thinh
On 2/7/2023 5:10 PM, Thinh Nguyen wrote:
> On Tue, Feb 07, 2023, Elson Serrao wrote:
>> On 2/6/2023 4:48 PM, Thinh Nguyen wrote:
>>>> +static int __dwc3_gadget_wakeup(struct dwc3 *dwc, bool async)
>>>> {
>>>> int retries;
>>>> @@ -2296,9 +2309,14 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
>>>> link_state = DWC3_DSTS_USBLNKST(reg);
>>>> switch (link_state) {
>>>> + case DWC3_LINK_STATE_U3: /* in HS, means SUSPEND */
>>>
>>> It's also possible to do remote wakeup in L1 for highspeed.
>>>
>>
>> The rw_configured flag here is in context of triggering remote wakeup from
>> bus suspend only.
>>
>> The remote wakeup setting for l1 in HighSpeed is controlled through LPM
>> token and overrides/ignores the config desc bmAttributes wakeup bit.
>>
>> Section 4.1 of USB2_LinkPowerMangement_ECN[final] spec "The host system sets the Remote Wake Flag parameter in this request to
>> enable or disable the addressed device
>> for remote wake from L1. The value of this flag will temporarily (while in
>> L1) override the current setting of the
>> Remote Wake feature settable by the standard Set/ClearFeature() commands
>> defined in Universal Serial Bus Specification, revision 2.0, Chapter 9."
>>
>> Please let me know if I am missing something.
>>
>
> It overrides the setting of the SetFeature request, not the device
> configuration.
>
> The rw_configured reflects the user configuration. Whether the host
> tries to enable the remote wakeup through SetFeature request or LPM
> token, the device should operate within the user configuration
> limitation.
>
> If the configuration indicates that it doesn't support remote wakeup, we
> should prevent unexpected behavior from the device. For simplicity, we
> can just return failure to wakeup for all states.
>
> Thanks,
> Thinh
L1 entry/exit is HW controlled and the remote wakeup is
conditional.(Section 7.1/Table7.2 of dwc3 data book). Even though we
block it from
SW the l1 exit will still happen from HW point of view.
To correlate the user configuration with LPM token, I experimented by
disabling the wakeup bit in the bmAtrributes, but I still see remote
wakeup bit being set in the LPM token. From the observation it seems
like there is no correlation between the wakeup bit in the bmAtrributes
and the wakeup bit in the LPM token.
Regards
Elson
On Tue, Feb 07, 2023, Elson Serrao wrote:
>
>
> On 2/7/2023 5:10 PM, Thinh Nguyen wrote:
> > On Tue, Feb 07, 2023, Elson Serrao wrote:
> > > On 2/6/2023 4:48 PM, Thinh Nguyen wrote:
> > > > > +static int __dwc3_gadget_wakeup(struct dwc3 *dwc, bool async)
> > > > > {
> > > > > int retries;
> > > > > @@ -2296,9 +2309,14 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
> > > > > link_state = DWC3_DSTS_USBLNKST(reg);
> > > > > switch (link_state) {
> > > > > + case DWC3_LINK_STATE_U3: /* in HS, means SUSPEND */
> > > >
> > > > It's also possible to do remote wakeup in L1 for highspeed.
> > > >
> > >
> > > The rw_configured flag here is in context of triggering remote wakeup from
> > > bus suspend only.
> > >
> > > The remote wakeup setting for l1 in HighSpeed is controlled through LPM
> > > token and overrides/ignores the config desc bmAttributes wakeup bit.
> > >
> > > Section 4.1 of USB2_LinkPowerMangement_ECN[final] spec "The host system sets the Remote Wake Flag parameter in this request to
> > > enable or disable the addressed device
> > > for remote wake from L1. The value of this flag will temporarily (while in
> > > L1) override the current setting of the
> > > Remote Wake feature settable by the standard Set/ClearFeature() commands
> > > defined in Universal Serial Bus Specification, revision 2.0, Chapter 9."
> > >
> > > Please let me know if I am missing something.
> > >
> >
> > It overrides the setting of the SetFeature request, not the device
> > configuration.
> >
> > The rw_configured reflects the user configuration. Whether the host
> > tries to enable the remote wakeup through SetFeature request or LPM
> > token, the device should operate within the user configuration
> > limitation.
> >
> > If the configuration indicates that it doesn't support remote wakeup, we
> > should prevent unexpected behavior from the device. For simplicity, we
> > can just return failure to wakeup for all states.
> >
> > Thanks,
> > Thinh
>
> L1 entry/exit is HW controlled and the remote wakeup is conditional.(Section
> 7.1/Table7.2 of dwc3 data book). Even though we block it from
> SW the l1 exit will still happen from HW point of view.
>
> To correlate the user configuration with LPM token, I experimented by
> disabling the wakeup bit in the bmAtrributes, but I still see remote wakeup
> bit being set in the LPM token. From the observation it seems like there is
That's because the linux xhci driver enables remote wakeup bit in its
port without regard for the device configuration.
> no correlation between the wakeup bit in the bmAtrributes and the wakeup bit
> in the LPM token.
>
The host can bring the device out of L1, that's probably what you saw.
The controller doesn't initiate remote wakeup by itself.
Thanks,
Thinh
On 2/7/2023 6:11 PM, Thinh Nguyen wrote:
> On Tue, Feb 07, 2023, Elson Serrao wrote:
>>
>>
>> On 2/7/2023 5:10 PM, Thinh Nguyen wrote:
>>> On Tue, Feb 07, 2023, Elson Serrao wrote:
>>>> On 2/6/2023 4:48 PM, Thinh Nguyen wrote:
>>>>>> +static int __dwc3_gadget_wakeup(struct dwc3 *dwc, bool async)
>>>>>> {
>>>>>> int retries;
>>>>>> @@ -2296,9 +2309,14 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
>>>>>> link_state = DWC3_DSTS_USBLNKST(reg);
>>>>>> switch (link_state) {
>>>>>> + case DWC3_LINK_STATE_U3: /* in HS, means SUSPEND */
>>>>>
>>>>> It's also possible to do remote wakeup in L1 for highspeed.
>>>>>
>>>>
>>>> The rw_configured flag here is in context of triggering remote wakeup from
>>>> bus suspend only.
>>>>
>>>> The remote wakeup setting for l1 in HighSpeed is controlled through LPM
>>>> token and overrides/ignores the config desc bmAttributes wakeup bit.
>>>>
>>>> Section 4.1 of USB2_LinkPowerMangement_ECN[final] spec "The host system sets the Remote Wake Flag parameter in this request to
>>>> enable or disable the addressed device
>>>> for remote wake from L1. The value of this flag will temporarily (while in
>>>> L1) override the current setting of the
>>>> Remote Wake feature settable by the standard Set/ClearFeature() commands
>>>> defined in Universal Serial Bus Specification, revision 2.0, Chapter 9."
>>>>
>>>> Please let me know if I am missing something.
>>>>
>>>
>>> It overrides the setting of the SetFeature request, not the device
>>> configuration.
>>>
>>> The rw_configured reflects the user configuration. Whether the host
>>> tries to enable the remote wakeup through SetFeature request or LPM
>>> token, the device should operate within the user configuration
>>> limitation.
>>>
>>> If the configuration indicates that it doesn't support remote wakeup, we
>>> should prevent unexpected behavior from the device. For simplicity, we
>>> can just return failure to wakeup for all states.
>>>
>>> Thanks,
>>> Thinh
>>
>> L1 entry/exit is HW controlled and the remote wakeup is conditional.(Section
>> 7.1/Table7.2 of dwc3 data book). Even though we block it from
>> SW the l1 exit will still happen from HW point of view.
>>
>> To correlate the user configuration with LPM token, I experimented by
>> disabling the wakeup bit in the bmAtrributes, but I still see remote wakeup
>> bit being set in the LPM token. From the observation it seems like there is
>
> That's because the linux xhci driver enables remote wakeup bit in its
> port without regard for the device configuration.
>
>> no correlation between the wakeup bit in the bmAtrributes and the wakeup bit
>> in the LPM token.
>>
>
> The host can bring the device out of L1, that's probably what you saw.
> The controller doesn't initiate remote wakeup by itself.
>
> Thanks,
> Thinh
Actually it seems the controller is initiating a remote wakeup by itself
to exit from l1 when we send a STARTTRANSFER command. I did below
experiment when the device was in HighSpeed
1.) Enabled l1.
2.) Disabled the remote wakeup software path (i.e avoid calling
__gadget_wakeup() if link is in l1 in the gadget_ep_cmd() path).
3.) Sent an IN packet when the link was in l1.
From the lecroy logs it looks like the controller initiated a remote
wakeup and sent the data.
Below are the events and the corresponding lecroy snippet
1.)Packet(55551) ------------> LPM token from Windows Host PC.
2.) Link in l1 for 2.445 secs
3. ) Send a ping data from device to host
4. )Packet(55554) ----------------> Resume
5.) IN data
Packet#
_______|_______________________________________________________________________
Transaction(26584) H(S) EXT(0x0F) LPM(0xC3) ADDR(11) ENDP(0) BESL(150 us)
_______| Link State(0x1) Rem Wake(0x1) ACK(0x4B) Time Stamp(27 . 204 671
632)
_______|_______________________________________________________________________Ch0
Packet(55550) Dir H(S) EXT(0x0F) ADDR(11) ENDP(0) CRC5(0x04) Pkt Len(8)
_______| Duration(133.330 ns) Idle(200.660 ns) Time Stamp(27 . 204 671 632)
_______|_______________________________________________________________________Ch0
Packet(55551) Dir H(S) LPM(0xC3) BESL(150 us) Link State(0x1)
_______| Rem Wake(0x1) Rsvd(0x0) CRC5(0x04) Pkt Len(8) Duration(133.330 ns)
_______| Idle(182.660 ns) Time Stamp(27 . 204 671 966)
_______|_______________________________________________________________________Ch0
Packet(55552) Dir H(S) ACK(0x4B) Pkt Len(6) Duration(100.000 ns)
_______| Idle( 11.450 us) Time Stamp(27 . 204 672 282)
_______|_______________________________________________________________________Ch0
Packet(55553) Dir(?) Full Speed J (Suspend)( 2.445 sec)
_______| Time Stamp(27 . 204 683 832)
_______|_______________________________________________________________________Ch0
Packet(55554) Dir(?) Full Speed K (Resume?)( 95.168 us) Time(165.134 us)
_______| Time Stamp(29 . 649 644 482)
_______|_______________________________________________________________________
Transfer(67) H(S) Bulk(IN) ADDR(11) ENDP(1) Bytes Transferred(142)
_______| Time(309.366 us) Time Stamp(29 . 649 809 616)
_______|_______________________________________________________________________
Transfer(68) H(S) Bulk(OUT) ADDR(11) ENDP(1) Bytes Transferred(142)
_______| Time(520.050 us) Time Stamp(29 . 650 118 982)
_______|_______________________________________________________________________
Transaction(26655) H(S) EXT(0x0F) LPM(0xC3) ADDR(11) ENDP(0) BESL(150 us)
_______| Link State(0x1) Rem Wake(0x1) ACK(0x4B) Time( 12.168 us)
_______| Time Stamp(29 . 650 639 032)
_______|_______________________________________________________________________
If software was solely responsible for waking up from l1, then there
should be no reason why the host would exit l1 in this scenario.
I tried with different ping intervals and saw that the duration for
which the link was in l1 correlates with the ping interval.
Thanks
Elson
On Thu, Feb 09, 2023, Elson Serrao wrote:
>
>
> On 2/7/2023 6:11 PM, Thinh Nguyen wrote:
> > On Tue, Feb 07, 2023, Elson Serrao wrote:
> > >
> > >
> > > On 2/7/2023 5:10 PM, Thinh Nguyen wrote:
> > > > On Tue, Feb 07, 2023, Elson Serrao wrote:
> > > > > On 2/6/2023 4:48 PM, Thinh Nguyen wrote:
> > > > > > > +static int __dwc3_gadget_wakeup(struct dwc3 *dwc, bool async)
> > > > > > > {
> > > > > > > int retries;
> > > > > > > @@ -2296,9 +2309,14 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
> > > > > > > link_state = DWC3_DSTS_USBLNKST(reg);
> > > > > > > switch (link_state) {
> > > > > > > + case DWC3_LINK_STATE_U3: /* in HS, means SUSPEND */
> > > > > >
> > > > > > It's also possible to do remote wakeup in L1 for highspeed.
> > > > > >
> > > > >
> > > > > The rw_configured flag here is in context of triggering remote wakeup from
> > > > > bus suspend only.
> > > > >
> > > > > The remote wakeup setting for l1 in HighSpeed is controlled through LPM
> > > > > token and overrides/ignores the config desc bmAttributes wakeup bit.
> > > > >
> > > > > Section 4.1 of USB2_LinkPowerMangement_ECN[final] spec "The host system sets the Remote Wake Flag parameter in this request to
> > > > > enable or disable the addressed device
> > > > > for remote wake from L1. The value of this flag will temporarily (while in
> > > > > L1) override the current setting of the
> > > > > Remote Wake feature settable by the standard Set/ClearFeature() commands
> > > > > defined in Universal Serial Bus Specification, revision 2.0, Chapter 9."
> > > > >
> > > > > Please let me know if I am missing something.
> > > > >
> > > >
> > > > It overrides the setting of the SetFeature request, not the device
> > > > configuration.
> > > >
> > > > The rw_configured reflects the user configuration. Whether the host
> > > > tries to enable the remote wakeup through SetFeature request or LPM
> > > > token, the device should operate within the user configuration
> > > > limitation.
> > > >
> > > > If the configuration indicates that it doesn't support remote wakeup, we
> > > > should prevent unexpected behavior from the device. For simplicity, we
> > > > can just return failure to wakeup for all states.
> > > >
> > > > Thanks,
> > > > Thinh
> > >
> > > L1 entry/exit is HW controlled and the remote wakeup is conditional.(Section
> > > 7.1/Table7.2 of dwc3 data book). Even though we block it from
> > > SW the l1 exit will still happen from HW point of view.
> > >
> > > To correlate the user configuration with LPM token, I experimented by
> > > disabling the wakeup bit in the bmAtrributes, but I still see remote wakeup
> > > bit being set in the LPM token. From the observation it seems like there is
> >
> > That's because the linux xhci driver enables remote wakeup bit in its
> > port without regard for the device configuration.
> >
> > > no correlation between the wakeup bit in the bmAtrributes and the wakeup bit
> > > in the LPM token.
> > >
> >
> > The host can bring the device out of L1, that's probably what you saw.
> > The controller doesn't initiate remote wakeup by itself.
> >
> > Thanks,
> > Thinh
>
> Actually it seems the controller is initiating a remote wakeup by itself to
> exit from l1 when we send a STARTTRANSFER command. I did below experiment
> when the device was in HighSpeed
>
That's driven by the driver telling the controller to initiate remote
wakeup and not the controller itself. When we send the START_TRANSFER
command, the driver does remote wakeup so the host would bring the
device to ON state so that the command can go through.
However you bring up a good point that if we prevent remote wakeup for
L1, then we have to delay sending START_TRANSFER command until the host
initiate resume. This would require additional enhancement to dwc3 to
handle this scenario. For now, can we ignore this specific case when
sending START_TRANSFER command and only check for the case when the user
trigger remote wakeup via gadget->ops->wakeup/func_wakeup.
Thanks,
Thinh
On 2/9/2023 6:27 PM, Thinh Nguyen wrote:
> On Thu, Feb 09, 2023, Elson Serrao wrote:
>>
>>
>> On 2/7/2023 6:11 PM, Thinh Nguyen wrote:
>>> On Tue, Feb 07, 2023, Elson Serrao wrote:
>>>>
>>>>
>>>> On 2/7/2023 5:10 PM, Thinh Nguyen wrote:
>>>>> On Tue, Feb 07, 2023, Elson Serrao wrote:
>>>>>> On 2/6/2023 4:48 PM, Thinh Nguyen wrote:
>>>>>>>> +static int __dwc3_gadget_wakeup(struct dwc3 *dwc, bool async)
>>>>>>>> {
>>>>>>>> int retries;
>>>>>>>> @@ -2296,9 +2309,14 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
>>>>>>>> link_state = DWC3_DSTS_USBLNKST(reg);
>>>>>>>> switch (link_state) {
>>>>>>>> + case DWC3_LINK_STATE_U3: /* in HS, means SUSPEND */
>>>>>>>
>>>>>>> It's also possible to do remote wakeup in L1 for highspeed.
>>>>>>>
>>>>>>
>>>>>> The rw_configured flag here is in context of triggering remote wakeup from
>>>>>> bus suspend only.
>>>>>>
>>>>>> The remote wakeup setting for l1 in HighSpeed is controlled through LPM
>>>>>> token and overrides/ignores the config desc bmAttributes wakeup bit.
>>>>>>
>>>>>> Section 4.1 of USB2_LinkPowerMangement_ECN[final] spec "The host system sets the Remote Wake Flag parameter in this request to
>>>>>> enable or disable the addressed device
>>>>>> for remote wake from L1. The value of this flag will temporarily (while in
>>>>>> L1) override the current setting of the
>>>>>> Remote Wake feature settable by the standard Set/ClearFeature() commands
>>>>>> defined in Universal Serial Bus Specification, revision 2.0, Chapter 9."
>>>>>>
>>>>>> Please let me know if I am missing something.
>>>>>>
>>>>>
>>>>> It overrides the setting of the SetFeature request, not the device
>>>>> configuration.
>>>>>
>>>>> The rw_configured reflects the user configuration. Whether the host
>>>>> tries to enable the remote wakeup through SetFeature request or LPM
>>>>> token, the device should operate within the user configuration
>>>>> limitation.
>>>>>
>>>>> If the configuration indicates that it doesn't support remote wakeup, we
>>>>> should prevent unexpected behavior from the device. For simplicity, we
>>>>> can just return failure to wakeup for all states.
>>>>>
>>>>> Thanks,
>>>>> Thinh
>>>>
>>>> L1 entry/exit is HW controlled and the remote wakeup is conditional.(Section
>>>> 7.1/Table7.2 of dwc3 data book). Even though we block it from
>>>> SW the l1 exit will still happen from HW point of view.
>>>>
>>>> To correlate the user configuration with LPM token, I experimented by
>>>> disabling the wakeup bit in the bmAtrributes, but I still see remote wakeup
>>>> bit being set in the LPM token. From the observation it seems like there is
>>>
>>> That's because the linux xhci driver enables remote wakeup bit in its
>>> port without regard for the device configuration.
>>>
>>>> no correlation between the wakeup bit in the bmAtrributes and the wakeup bit
>>>> in the LPM token.
>>>>
>>>
>>> The host can bring the device out of L1, that's probably what you saw.
>>> The controller doesn't initiate remote wakeup by itself.
>>>
>>> Thanks,
>>> Thinh
>>
>> Actually it seems the controller is initiating a remote wakeup by itself to
>> exit from l1 when we send a STARTTRANSFER command. I did below experiment
>> when the device was in HighSpeed
>>
>
> That's driven by the driver telling the controller to initiate remote
> wakeup and not the controller itself. When we send the START_TRANSFER
> command, the driver does remote wakeup so the host would bring the
> device to ON state so that the command can go through.
>
> However you bring up a good point that if we prevent remote wakeup for
> L1, then we have to delay sending START_TRANSFER command until the host
> initiate resume. This would require additional enhancement to dwc3 to
> handle this scenario. For now, can we ignore this specific case when
> sending START_TRANSFER command and only check for the case when the user
> trigger remote wakeup via gadget->ops->wakeup/func_wakeup.
>
> Thanks,
> Thinh
Sure. I will upload v4 with the suggested feedback/comments.
Thanks
Elson