[PATCH v3 0/3] Support dwc3 runtime suspend during bus suspend

[PATCH v3 0/3] Support dwc3 runtime suspend during bus suspend

[Elson Roy Serrao]

Changes in v3
 - Added a dt property 'snps,allow-rtsusp-on-u3' to make this feature platform
   dependent as per the feedback from Thinh N.
 - Changed the RT idle/suspend/resume handling to device mode specific and dt
   property dependent.
 - Modified the cover letter to document how resume is handled on qcom platforms.
 
Changes in v2
 - Used pm_runtime_resume_and_get() API instead of pm_runtime_get_sync()
   as suggested by Dan.
 - Handled the return value in ether_wakeup_host to print error message.

When a USB link is idle, the host sends a bus suspend event to the device
so that the device can save power. But true power savings during bus
suspend can be seen only if we let the USB controller enter low power
mode and turn off the clocks. Vendor drivers may have their own runtime
power management framework to power up/down the controller. But since
vendor drivers' runtime suspend/resume routines depend on the dwc3 child
node we would need a framework to trigger dwc3 runtime pm ops whenever a
bus suspend is received. If the device wants to exit from bus suspend
state it can send a wakeup signal to the host by first bringing out the
controller from low power mode. This series implements the needed
framework to achieve this functionality when a bus suspend interupt is
received. The assumption here is that the dwc3 hibernation feature is not
enabled and the platform is responsible in detecting the resume events to
bring the controller out of suspend.

On Qualcomm platforms the bus resume is handled through Phy and informed to
software through wakeup capable phy interrupts.
usb2 PHY is configured to detect the Resume K event and sends an interrupt
when this event is detected. This would trigger the runtime resume of the
glue driver which would intrinsically wakeup the dwc3 child. In case of usb3 PHY,
it is configured to detect the LFPS wake signal during bus resume and the
corresponding interrupt triggers the runtime resume of the glue driver.

The series is organized in below fashion:
Patch 1: This includes the modification needed from function drivers to let
UDC enter low power mode with u_ether as an example.
Patch 2: New dt property to allow dwc3 runtime suspedn during bus suspend scenario. 
Patch 3: This has the modification needed in the UDC driver to trigger runtime
suspend whene a bus suspend interrupt is received. Since this is a platform
dependent change it is made applicable through a dt property. This also handles
resume and remote wakeup modifications from power management perspective.

Elson Roy Serrao (3):
  usb: function: u_ether: Handle rx requests during suspend/resume
  dt-bindings: usb: snps,dwc3: Add allow-rtsusp-on-u3 property
  usb: dwc3: Modify runtime pm ops to handle bus suspend

 .../devicetree/bindings/usb/snps,dwc3.yaml    |  5 ++
 drivers/usb/dwc3/core.c                       | 26 ++++++++--
 drivers/usb/dwc3/core.h                       |  3 ++
 drivers/usb/dwc3/gadget.c                     | 40 +++++++++++++---
 drivers/usb/gadget/function/u_ether.c         | 47 +++++++++++++++----
 5 files changed, 102 insertions(+), 19 deletions(-)

-- 
2.17.1

[PATCH v3 1/3] usb: function: u_ether: Handle rx requests during suspend/resume

[Elson Roy Serrao]

Some UDCs might have a vote against runtime suspend if there is any
request queued by the function driver. This would block the UDC driver
to enter runtime suspend state when the host sends bus suspend
notification. While tx requests get dequeued after completion, rx
requests always remain queued for the next OUT data to be handled. Since
during bus suspend scenario there are no active OUT transfers we can
dequeue these requests when the function driver suspend callback gets
called and queue them back during resume callback. Implement this
mechanism by adding a new list for queued requests.

Also move the gether_wakeup_host API to work queue context to better
align with the remote wakeup op's synchronous operation.

Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
---
 drivers/usb/gadget/function/u_ether.c | 47 ++++++++++++++++++++++-----
 1 file changed, 38 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/gadget/function/u_ether.c b/drivers/usb/gadget/function/u_ether.c
index a366abb45623..107677b7656f 100644
--- a/drivers/usb/gadget/function/u_ether.c
+++ b/drivers/usb/gadget/function/u_ether.c
@@ -62,7 +62,7 @@ struct eth_dev {
 	struct usb_gadget	*gadget;
 
 	spinlock_t		req_lock;	/* guard {rx,tx}_reqs */
-	struct list_head	tx_reqs, rx_reqs;
+	struct list_head	tx_reqs, rx_reqs, rx_queued_reqs;
 	atomic_t		tx_qlen;
 
 	struct sk_buff_head	rx_frames;
@@ -75,7 +75,7 @@ struct eth_dev {
 						struct sk_buff *skb,
 						struct sk_buff_head *list);
 
-	struct work_struct	work;
+	struct work_struct	work, wakeup_work;
 
 	unsigned long		todo;
 #define	WORK_RX_MEMORY		0
@@ -213,7 +213,7 @@ rx_submit(struct eth_dev *dev, struct usb_request *req, gfp_t gfp_flags)
 		if (skb)
 			dev_kfree_skb_any(skb);
 		spin_lock_irqsave(&dev->req_lock, flags);
-		list_add(&req->list, &dev->rx_reqs);
+		list_move_tail(&req->list, &dev->rx_reqs);
 		spin_unlock_irqrestore(&dev->req_lock, flags);
 	}
 	return retval;
@@ -303,7 +303,7 @@ static void rx_complete(struct usb_ep *ep, struct usb_request *req)
 	if (!netif_running(dev->net)) {
 clean:
 		spin_lock(&dev->req_lock);
-		list_add(&req->list, &dev->rx_reqs);
+		list_move_tail(&req->list, &dev->rx_reqs);
 		spin_unlock(&dev->req_lock);
 		req = NULL;
 	}
@@ -378,7 +378,7 @@ static void rx_fill(struct eth_dev *dev, gfp_t gfp_flags)
 	spin_lock_irqsave(&dev->req_lock, flags);
 	while (!list_empty(&dev->rx_reqs)) {
 		req = list_first_entry(&dev->rx_reqs, struct usb_request, list);
-		list_del_init(&req->list);
+		list_move_tail(&req->list, &dev->rx_queued_reqs);
 		spin_unlock_irqrestore(&dev->req_lock, flags);
 
 		if (rx_submit(dev, req, gfp_flags) < 0) {
@@ -438,9 +438,11 @@ static inline int is_promisc(u16 cdc_filter)
 	return cdc_filter & USB_CDC_PACKET_TYPE_PROMISCUOUS;
 }
 
-static int ether_wakeup_host(struct gether *port)
+static void ether_wakeup_work(struct work_struct *w)
 {
 	int			ret;
+	struct eth_dev		*dev = container_of(w, struct eth_dev, wakeup_work);
+	struct gether		*port = dev->port_usb;
 	struct usb_function	*func = &port->func;
 	struct usb_gadget	*gadget = func->config->cdev->gadget;
 
@@ -449,7 +451,8 @@ static int ether_wakeup_host(struct gether *port)
 	else
 		ret = usb_gadget_wakeup(gadget);
 
-	return ret;
+	if (ret)
+		DBG(dev, "failed to trigger wakeup:%d\n", ret);
 }
 
 static netdev_tx_t eth_start_xmit(struct sk_buff *skb,
@@ -476,7 +479,7 @@ static netdev_tx_t eth_start_xmit(struct sk_buff *skb,
 		DBG(dev, "Port suspended. Triggering wakeup\n");
 		netif_stop_queue(net);
 		spin_unlock_irqrestore(&dev->lock, flags);
-		ether_wakeup_host(dev->port_usb);
+		schedule_work(&dev->wakeup_work);
 		return NETDEV_TX_BUSY;
 	}
 
@@ -754,8 +757,10 @@ struct eth_dev *gether_setup_name(struct usb_gadget *g,
 	spin_lock_init(&dev->lock);
 	spin_lock_init(&dev->req_lock);
 	INIT_WORK(&dev->work, eth_work);
+	INIT_WORK(&dev->wakeup_work, ether_wakeup_work);
 	INIT_LIST_HEAD(&dev->tx_reqs);
 	INIT_LIST_HEAD(&dev->rx_reqs);
+	INIT_LIST_HEAD(&dev->rx_queued_reqs);
 
 	skb_queue_head_init(&dev->rx_frames);
 
@@ -825,8 +830,10 @@ struct net_device *gether_setup_name_default(const char *netname)
 	spin_lock_init(&dev->lock);
 	spin_lock_init(&dev->req_lock);
 	INIT_WORK(&dev->work, eth_work);
+	INIT_WORK(&dev->wakeup_work, ether_wakeup_work);
 	INIT_LIST_HEAD(&dev->tx_reqs);
 	INIT_LIST_HEAD(&dev->rx_reqs);
+	INIT_LIST_HEAD(&dev->rx_queued_reqs);
 
 	skb_queue_head_init(&dev->rx_frames);
 
@@ -1043,6 +1050,7 @@ EXPORT_SYMBOL_GPL(gether_set_ifname);
 void gether_suspend(struct gether *link)
 {
 	struct eth_dev *dev = link->ioport;
+	struct usb_request *req;
 	unsigned long flags;
 
 	if (!dev)
@@ -1053,9 +1061,20 @@ void gether_suspend(struct gether *link)
 		 * There is a transfer in progress. So we trigger a remote
 		 * wakeup to inform the host.
 		 */
-		ether_wakeup_host(dev->port_usb);
+		schedule_work(&dev->wakeup_work);
 		return;
 	}
+	/* Dequeue the submitted requests. */
+	spin_lock(&dev->req_lock);
+	while (!list_empty(&dev->rx_queued_reqs)) {
+		req = list_last_entry(&dev->rx_queued_reqs, struct usb_request, list);
+		list_move_tail(&req->list, &dev->rx_reqs);
+		spin_unlock(&dev->req_lock);
+		usb_ep_dequeue(dev->port_usb->out_ep, req);
+		spin_lock(&dev->req_lock);
+	}
+	spin_unlock(&dev->req_lock);
+
 	spin_lock_irqsave(&dev->lock, flags);
 	link->is_suspend = true;
 	spin_unlock_irqrestore(&dev->lock, flags);
@@ -1070,6 +1089,7 @@ void gether_resume(struct gether *link)
 	if (!dev)
 		return;
 
+	defer_kevent(dev, WORK_RX_MEMORY);
 	if (netif_queue_stopped(dev->net))
 		netif_start_queue(dev->net);
 
@@ -1231,6 +1251,15 @@ void gether_disconnect(struct gether *link)
 		usb_ep_free_request(link->out_ep, req);
 		spin_lock(&dev->req_lock);
 	}
+
+	while (!list_empty(&dev->rx_queued_reqs)) {
+		req = list_first_entry(&dev->rx_queued_reqs, struct usb_request, list);
+		list_del(&req->list);
+
+		spin_unlock(&dev->req_lock);
+		usb_ep_free_request(link->out_ep, req);
+		spin_lock(&dev->req_lock);
+	}
 	spin_unlock(&dev->req_lock);
 	link->out_ep->desc = NULL;
 
-- 
2.17.1

[PATCH v3 2/3] dt-bindings: usb: snps,dwc3: Add allow-rtsusp-on-u3 property

[Elson Roy Serrao]

This property allows dwc3 runtime suspend when bus suspend interrupt
is received even with cable connected. This would allow the dwc3
controller to enter low power mode during bus suspend scenario.

This property would particularly benefit dwc3 IPs where hibernation is
not enabled and the dwc3 low power mode entry/exit is handled by the
glue driver. The assumption here is that the platform using this dt
property is capable of detecting resume events to bring the controller
out of suspend.

Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
---
 Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
index a696f23730d3..18ad99a26dd9 100644
--- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
+++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
@@ -403,6 +403,11 @@ properties:
     description:
       Enable USB remote wakeup.
 
+  snps,allow-rtsusp-on-u3:
+    description:
+      If True then dwc3 runtime suspend is allowed during bus suspend
+      case even with the USB cable connected.
+
 unevaluatedProperties: false
 
 required:
-- 
2.17.1

[PATCH v3 3/3] usb: dwc3: Modify runtime pm ops to handle bus suspend

[Elson Roy Serrao]

The current implementation blocks the runtime pm operations when cable
is connected. This would block platforms from entering system wide suspend
during bus suspend scenario. Modify the runtime pm ops to handle bus
suspend case for such platforms where the controller low power mode
entry/exit is handled by the glue driver. This enablement is controlled
through a dt property and platforms capable of detecting bus resume can
benefit from this feature. Also modify the remote wakeup operations to
trigger runtime resume before sending wakeup signal.

Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
---
 drivers/usb/dwc3/core.c   | 26 ++++++++++++++++++++++---
 drivers/usb/dwc3/core.h   |  3 +++
 drivers/usb/dwc3/gadget.c | 40 ++++++++++++++++++++++++++++++++-------
 3 files changed, 59 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index f6689b731718..898c0f68e190 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1534,6 +1534,9 @@ static void dwc3_get_properties(struct dwc3 *dwc)
 	dwc->dis_split_quirk = device_property_read_bool(dev,
 				"snps,dis-split-quirk");
 
+	dwc->allow_rtsusp_on_u3 = device_property_read_bool(dev,
+				"snps,allow-rtsusp-on-u3");
+
 	dwc->lpm_nyet_threshold = lpm_nyet_threshold;
 	dwc->tx_de_emphasis = tx_de_emphasis;
 
@@ -1984,11 +1987,21 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
 {
 	unsigned long	flags;
 	u32 reg;
+	int link_state;
 
 	switch (dwc->current_dr_role) {
 	case DWC3_GCTL_PRTCAP_DEVICE:
 		if (pm_runtime_suspended(dwc->dev))
 			break;
+
+		if (dwc->connected) {
+			link_state = dwc3_gadget_get_link_state(dwc);
+			/* bus suspend case */
+			if (dwc->allow_rtsusp_on_u3 &&
+			    link_state == DWC3_LINK_STATE_U3)
+				break;
+			return -EBUSY;
+		}
 		dwc3_gadget_suspend(dwc);
 		synchronize_irq(dwc->irq_gadget);
 		dwc3_core_exit(dwc);
@@ -2045,6 +2058,9 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
 
 	switch (dwc->current_dr_role) {
 	case DWC3_GCTL_PRTCAP_DEVICE:
+		/* bus resume case */
+		if (dwc->connected)
+			break;
 		ret = dwc3_core_init_for_resume(dwc);
 		if (ret)
 			return ret;
@@ -2123,9 +2139,6 @@ static int dwc3_runtime_suspend(struct device *dev)
 	struct dwc3     *dwc = dev_get_drvdata(dev);
 	int		ret;
 
-	if (dwc3_runtime_checks(dwc))
-		return -EBUSY;
-
 	ret = dwc3_suspend_common(dwc, PMSG_AUTO_SUSPEND);
 	if (ret)
 		return ret;
@@ -2160,9 +2173,15 @@ static int dwc3_runtime_resume(struct device *dev)
 static int dwc3_runtime_idle(struct device *dev)
 {
 	struct dwc3     *dwc = dev_get_drvdata(dev);
+	int		link_state;
 
 	switch (dwc->current_dr_role) {
 	case DWC3_GCTL_PRTCAP_DEVICE:
+		link_state = dwc3_gadget_get_link_state(dwc);
+		/* for bus suspend case return success */
+		if (dwc->allow_rtsusp_on_u3 && dwc->connected &&
+		    link_state == DWC3_LINK_STATE_U3)
+			goto autosuspend;
 		if (dwc3_runtime_checks(dwc))
 			return -EBUSY;
 		break;
@@ -2172,6 +2191,7 @@ static int dwc3_runtime_idle(struct device *dev)
 		break;
 	}
 
+autosuspend:
 	pm_runtime_mark_last_busy(dev);
 	pm_runtime_autosuspend(dev);
 
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 8b1295e4dcdd..33b2ccbbd963 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -1127,6 +1127,8 @@ struct dwc3_scratchpad_array {
  * @num_ep_resized: carries the current number endpoints which have had its tx
  *		    fifo resized.
  * @debug_root: root debugfs directory for this device to put its files in.
+ * @allow_rtsusp_on_u3: true if dwc3 runtime suspend is allowed during bus
+ *			suspend scenario.
  */
 struct dwc3 {
 	struct work_struct	drd_work;
@@ -1343,6 +1345,7 @@ struct dwc3 {
 	int			last_fifo_depth;
 	int			num_ep_resized;
 	struct dentry		*debug_root;
+	bool			allow_rtsusp_on_u3;
 };
 
 #define INCRX_BURST_MODE 0
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 5fd067151fbf..0797cffa2d48 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2401,15 +2401,21 @@ static int dwc3_gadget_wakeup(struct usb_gadget *g)
 		return -EINVAL;
 	}
 
-	spin_lock_irqsave(&dwc->lock, flags);
 	if (!dwc->gadget->wakeup_armed) {
 		dev_err(dwc->dev, "not armed for remote wakeup\n");
-		spin_unlock_irqrestore(&dwc->lock, flags);
 		return -EINVAL;
 	}
-	ret = __dwc3_gadget_wakeup(dwc, true);
 
+	ret = pm_runtime_resume_and_get(dwc->dev);
+	if (ret < 0) {
+		pm_runtime_set_suspended(dwc->dev);
+		return ret;
+	}
+
+	spin_lock_irqsave(&dwc->lock, flags);
+	ret = __dwc3_gadget_wakeup(dwc, true);
 	spin_unlock_irqrestore(&dwc->lock, flags);
+	pm_runtime_put_noidle(dwc->dev);
 
 	return ret;
 }
@@ -2428,6 +2434,12 @@ static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int intf_id)
 		return -EINVAL;
 	}
 
+	ret = pm_runtime_resume_and_get(dwc->dev);
+	if (ret < 0) {
+		pm_runtime_set_suspended(dwc->dev);
+		return ret;
+	}
+
 	spin_lock_irqsave(&dwc->lock, flags);
 	/*
 	 * If the link is in U3, signal for remote wakeup and wait for the
@@ -2438,6 +2450,7 @@ static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int intf_id)
 		ret = __dwc3_gadget_wakeup(dwc, false);
 		if (ret) {
 			spin_unlock_irqrestore(&dwc->lock, flags);
+			pm_runtime_put_noidle(dwc->dev);
 			return -EINVAL;
 		}
 		dwc3_resume_gadget(dwc);
@@ -2452,6 +2465,7 @@ static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int intf_id)
 		dev_err(dwc->dev, "function remote wakeup failed, ret:%d\n", ret);
 
 	spin_unlock_irqrestore(&dwc->lock, flags);
+	pm_runtime_put_noidle(dwc->dev);
 
 	return ret;
 }
@@ -2732,21 +2746,23 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
 	/*
 	 * Avoid issuing a runtime resume if the device is already in the
 	 * suspended state during gadget disconnect.  DWC3 gadget was already
-	 * halted/stopped during runtime suspend.
+	 * halted/stopped during runtime suspend except for bus suspend case
+	 * where we would have skipped the controller halt.
 	 */
 	if (!is_on) {
 		pm_runtime_barrier(dwc->dev);
-		if (pm_runtime_suspended(dwc->dev))
+		if (pm_runtime_suspended(dwc->dev) && !dwc->connected)
 			return 0;
 	}
 
 	/*
 	 * Check the return value for successful resume, or error.  For a
 	 * successful resume, the DWC3 runtime PM resume routine will handle
-	 * the run stop sequence, so avoid duplicate operations here.
+	 * the run stop sequence except for bus resume case, so avoid
+	 * duplicate operations here.
 	 */
 	ret = pm_runtime_get_sync(dwc->dev);
-	if (!ret || ret < 0) {
+	if ((!ret && !dwc->connected) || ret < 0) {
 		pm_runtime_put(dwc->dev);
 		if (ret < 0)
 			pm_runtime_set_suspended(dwc->dev);
@@ -4331,6 +4347,8 @@ static void dwc3_gadget_suspend_interrupt(struct dwc3 *dwc,
 	}
 
 	dwc->link_state = next;
+	pm_runtime_mark_last_busy(dwc->dev);
+	pm_request_autosuspend(dwc->dev);
 }
 
 static void dwc3_gadget_interrupt(struct dwc3 *dwc,
@@ -4718,7 +4736,15 @@ void dwc3_gadget_process_pending_events(struct dwc3 *dwc)
 {
 	if (dwc->pending_events) {
 		dwc3_interrupt(dwc->irq_gadget, dwc->ev_buf);
+		pm_runtime_put(dwc->dev);
 		dwc->pending_events = false;
 		enable_irq(dwc->irq_gadget);
+		/*
+		 * We have only stored the pending events as part
+		 * of dwc3_interrupt() above, but those events are
+		 * not yet handled. So explicitly invoke the
+		 * interrupt handler for handling those events.
+		 */
+		dwc3_thread_interrupt(dwc->irq_gadget, dwc->ev_buf);
 	}
 }
-- 
2.17.1

Re: [PATCH v3 2/3] dt-bindings: usb: snps,dwc3: Add allow-rtsusp-on-u3 property

[Thinh Nguyen]

On Tue, Jul 11, 2023, Elson Roy Serrao wrote:
> This property allows dwc3 runtime suspend when bus suspend interrupt
> is received even with cable connected. This would allow the dwc3
> controller to enter low power mode during bus suspend scenario.
> 
> This property would particularly benefit dwc3 IPs where hibernation is
> not enabled and the dwc3 low power mode entry/exit is handled by the
> glue driver. The assumption here is that the platform using this dt
> property is capable of detecting resume events to bring the controller
> out of suspend.
> 
> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
> ---
>  Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> index a696f23730d3..18ad99a26dd9 100644
> --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> @@ -403,6 +403,11 @@ properties:
>      description:
>        Enable USB remote wakeup.
>  
> +  snps,allow-rtsusp-on-u3:

Please spell out the whole thing as "rtsusp" isn't clear. Also, it's not
just for U3 right? For highspeed, it's L2.

How about the name that Roger use: "snps,gadget-keep-connect-sys-sleep"

> +    description:
> +      If True then dwc3 runtime suspend is allowed during bus suspend
> +      case even with the USB cable connected.
> +
>  unevaluatedProperties: false
>  
>  required:
> -- 
> 2.17.1
> 

Did you Cc Rob, the devicetree maintainer?

Thanks,
Thinh

Re: [PATCH v3 0/3] Support dwc3 runtime suspend during bus suspend

[Thinh Nguyen]

On Tue, Jul 11, 2023, Elson Roy Serrao wrote:
> Changes in v3
>  - Added a dt property 'snps,allow-rtsusp-on-u3' to make this feature platform
>    dependent as per the feedback from Thinh N.
>  - Changed the RT idle/suspend/resume handling to device mode specific and dt
>    property dependent.
>  - Modified the cover letter to document how resume is handled on qcom platforms.
>  
> Changes in v2
>  - Used pm_runtime_resume_and_get() API instead of pm_runtime_get_sync()
>    as suggested by Dan.
>  - Handled the return value in ether_wakeup_host to print error message.
> 
> When a USB link is idle, the host sends a bus suspend event to the device
> so that the device can save power. But true power savings during bus
> suspend can be seen only if we let the USB controller enter low power
> mode and turn off the clocks. Vendor drivers may have their own runtime
> power management framework to power up/down the controller. But since
> vendor drivers' runtime suspend/resume routines depend on the dwc3 child
> node we would need a framework to trigger dwc3 runtime pm ops whenever a
> bus suspend is received. If the device wants to exit from bus suspend
> state it can send a wakeup signal to the host by first bringing out the
> controller from low power mode. This series implements the needed
> framework to achieve this functionality when a bus suspend interupt is
> received. The assumption here is that the dwc3 hibernation feature is not
> enabled and the platform is responsible in detecting the resume events to
> bring the controller out of suspend.
> 
> On Qualcomm platforms the bus resume is handled through Phy and informed to
> software through wakeup capable phy interrupts.
> usb2 PHY is configured to detect the Resume K event and sends an interrupt
> when this event is detected. This would trigger the runtime resume of the
> glue driver which would intrinsically wakeup the dwc3 child. In case of usb3 PHY,
> it is configured to detect the LFPS wake signal during bus resume and the
> corresponding interrupt triggers the runtime resume of the glue driver.

Thanks for the info confirming that it's capable for both usb3 and usb2.

BR,
Thinh

> 
> The series is organized in below fashion:
> Patch 1: This includes the modification needed from function drivers to let
> UDC enter low power mode with u_ether as an example.
> Patch 2: New dt property to allow dwc3 runtime suspedn during bus suspend scenario. 
> Patch 3: This has the modification needed in the UDC driver to trigger runtime
> suspend whene a bus suspend interrupt is received. Since this is a platform
> dependent change it is made applicable through a dt property. This also handles
> resume and remote wakeup modifications from power management perspective.
> 
> Elson Roy Serrao (3):
>   usb: function: u_ether: Handle rx requests during suspend/resume
>   dt-bindings: usb: snps,dwc3: Add allow-rtsusp-on-u3 property
>   usb: dwc3: Modify runtime pm ops to handle bus suspend
> 
>  .../devicetree/bindings/usb/snps,dwc3.yaml    |  5 ++
>  drivers/usb/dwc3/core.c                       | 26 ++++++++--
>  drivers/usb/dwc3/core.h                       |  3 ++
>  drivers/usb/dwc3/gadget.c                     | 40 +++++++++++++---
>  drivers/usb/gadget/function/u_ether.c         | 47 +++++++++++++++----
>  5 files changed, 102 insertions(+), 19 deletions(-)
> 
> -- 
> 2.17.1
> 

Re: [PATCH v3 3/3] usb: dwc3: Modify runtime pm ops to handle bus suspend

[Thinh Nguyen]

On Tue, Jul 11, 2023, Elson Roy Serrao wrote:
> The current implementation blocks the runtime pm operations when cable
> is connected. This would block platforms from entering system wide suspend
> during bus suspend scenario. Modify the runtime pm ops to handle bus
> suspend case for such platforms where the controller low power mode
> entry/exit is handled by the glue driver. This enablement is controlled
> through a dt property and platforms capable of detecting bus resume can
> benefit from this feature. Also modify the remote wakeup operations to
> trigger runtime resume before sending wakeup signal.
> 
> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
> ---
>  drivers/usb/dwc3/core.c   | 26 ++++++++++++++++++++++---
>  drivers/usb/dwc3/core.h   |  3 +++
>  drivers/usb/dwc3/gadget.c | 40 ++++++++++++++++++++++++++++++++-------
>  3 files changed, 59 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index f6689b731718..898c0f68e190 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1534,6 +1534,9 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>  	dwc->dis_split_quirk = device_property_read_bool(dev,
>  				"snps,dis-split-quirk");
>  
> +	dwc->allow_rtsusp_on_u3 = device_property_read_bool(dev,
> +				"snps,allow-rtsusp-on-u3");
> +
>  	dwc->lpm_nyet_threshold = lpm_nyet_threshold;
>  	dwc->tx_de_emphasis = tx_de_emphasis;
>  
> @@ -1984,11 +1987,21 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
>  {
>  	unsigned long	flags;
>  	u32 reg;
> +	int link_state;
>  
>  	switch (dwc->current_dr_role) {
>  	case DWC3_GCTL_PRTCAP_DEVICE:
>  		if (pm_runtime_suspended(dwc->dev))
>  			break;
> +
> +		if (dwc->connected) {
> +			link_state = dwc3_gadget_get_link_state(dwc);
> +			/* bus suspend case */
> +			if (dwc->allow_rtsusp_on_u3 &&
> +			    link_state == DWC3_LINK_STATE_U3)
> +				break;
> +			return -EBUSY;
> +		}
>  		dwc3_gadget_suspend(dwc);
>  		synchronize_irq(dwc->irq_gadget);
>  		dwc3_core_exit(dwc);
> @@ -2045,6 +2058,9 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
>  
>  	switch (dwc->current_dr_role) {
>  	case DWC3_GCTL_PRTCAP_DEVICE:
> +		/* bus resume case */
> +		if (dwc->connected)
> +			break;
>  		ret = dwc3_core_init_for_resume(dwc);
>  		if (ret)
>  			return ret;
> @@ -2123,9 +2139,6 @@ static int dwc3_runtime_suspend(struct device *dev)
>  	struct dwc3     *dwc = dev_get_drvdata(dev);
>  	int		ret;
>  
> -	if (dwc3_runtime_checks(dwc))
> -		return -EBUSY;
> -
>  	ret = dwc3_suspend_common(dwc, PMSG_AUTO_SUSPEND);
>  	if (ret)
>  		return ret;
> @@ -2160,9 +2173,15 @@ static int dwc3_runtime_resume(struct device *dev)
>  static int dwc3_runtime_idle(struct device *dev)
>  {
>  	struct dwc3     *dwc = dev_get_drvdata(dev);
> +	int		link_state;
>  
>  	switch (dwc->current_dr_role) {
>  	case DWC3_GCTL_PRTCAP_DEVICE:
> +		link_state = dwc3_gadget_get_link_state(dwc);
> +		/* for bus suspend case return success */
> +		if (dwc->allow_rtsusp_on_u3 && dwc->connected &&
> +		    link_state == DWC3_LINK_STATE_U3)
> +			goto autosuspend;
>  		if (dwc3_runtime_checks(dwc))
>  			return -EBUSY;
>  		break;
> @@ -2172,6 +2191,7 @@ static int dwc3_runtime_idle(struct device *dev)
>  		break;
>  	}
>  
> +autosuspend:
>  	pm_runtime_mark_last_busy(dev);
>  	pm_runtime_autosuspend(dev);
>  
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 8b1295e4dcdd..33b2ccbbd963 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -1127,6 +1127,8 @@ struct dwc3_scratchpad_array {
>   * @num_ep_resized: carries the current number endpoints which have had its tx
>   *		    fifo resized.
>   * @debug_root: root debugfs directory for this device to put its files in.
> + * @allow_rtsusp_on_u3: true if dwc3 runtime suspend is allowed during bus
> + *			suspend scenario.
>   */
>  struct dwc3 {
>  	struct work_struct	drd_work;
> @@ -1343,6 +1345,7 @@ struct dwc3 {
>  	int			last_fifo_depth;
>  	int			num_ep_resized;
>  	struct dentry		*debug_root;
> +	bool			allow_rtsusp_on_u3;
>  };
>  
>  #define INCRX_BURST_MODE 0
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 5fd067151fbf..0797cffa2d48 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2401,15 +2401,21 @@ static int dwc3_gadget_wakeup(struct usb_gadget *g)
>  		return -EINVAL;
>  	}
>  
> -	spin_lock_irqsave(&dwc->lock, flags);
>  	if (!dwc->gadget->wakeup_armed) {
>  		dev_err(dwc->dev, "not armed for remote wakeup\n");
> -		spin_unlock_irqrestore(&dwc->lock, flags);
>  		return -EINVAL;
>  	}
> -	ret = __dwc3_gadget_wakeup(dwc, true);
>  
> +	ret = pm_runtime_resume_and_get(dwc->dev);
> +	if (ret < 0) {
> +		pm_runtime_set_suspended(dwc->dev);
> +		return ret;
> +	}
> +
> +	spin_lock_irqsave(&dwc->lock, flags);
> +	ret = __dwc3_gadget_wakeup(dwc, true);
>  	spin_unlock_irqrestore(&dwc->lock, flags);
> +	pm_runtime_put_noidle(dwc->dev);
>  
>  	return ret;
>  }
> @@ -2428,6 +2434,12 @@ static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int intf_id)
>  		return -EINVAL;
>  	}
>  
> +	ret = pm_runtime_resume_and_get(dwc->dev);
> +	if (ret < 0) {
> +		pm_runtime_set_suspended(dwc->dev);
> +		return ret;
> +	}
> +
>  	spin_lock_irqsave(&dwc->lock, flags);
>  	/*
>  	 * If the link is in U3, signal for remote wakeup and wait for the
> @@ -2438,6 +2450,7 @@ static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int intf_id)
>  		ret = __dwc3_gadget_wakeup(dwc, false);
>  		if (ret) {
>  			spin_unlock_irqrestore(&dwc->lock, flags);
> +			pm_runtime_put_noidle(dwc->dev);
>  			return -EINVAL;
>  		}
>  		dwc3_resume_gadget(dwc);
> @@ -2452,6 +2465,7 @@ static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int intf_id)
>  		dev_err(dwc->dev, "function remote wakeup failed, ret:%d\n", ret);
>  
>  	spin_unlock_irqrestore(&dwc->lock, flags);
> +	pm_runtime_put_noidle(dwc->dev);
>  
>  	return ret;
>  }
> @@ -2732,21 +2746,23 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
>  	/*
>  	 * Avoid issuing a runtime resume if the device is already in the
>  	 * suspended state during gadget disconnect.  DWC3 gadget was already
> -	 * halted/stopped during runtime suspend.
> +	 * halted/stopped during runtime suspend except for bus suspend case
> +	 * where we would have skipped the controller halt.
>  	 */
>  	if (!is_on) {
>  		pm_runtime_barrier(dwc->dev);
> -		if (pm_runtime_suspended(dwc->dev))
> +		if (pm_runtime_suspended(dwc->dev) && !dwc->connected)
>  			return 0;
>  	}
>  
>  	/*
>  	 * Check the return value for successful resume, or error.  For a
>  	 * successful resume, the DWC3 runtime PM resume routine will handle
> -	 * the run stop sequence, so avoid duplicate operations here.
> +	 * the run stop sequence except for bus resume case, so avoid
> +	 * duplicate operations here.
>  	 */
>  	ret = pm_runtime_get_sync(dwc->dev);
> -	if (!ret || ret < 0) {
> +	if ((!ret && !dwc->connected) || ret < 0) {
>  		pm_runtime_put(dwc->dev);
>  		if (ret < 0)
>  			pm_runtime_set_suspended(dwc->dev);
> @@ -4331,6 +4347,8 @@ static void dwc3_gadget_suspend_interrupt(struct dwc3 *dwc,
>  	}
>  
>  	dwc->link_state = next;
> +	pm_runtime_mark_last_busy(dwc->dev);
> +	pm_request_autosuspend(dwc->dev);
>  }
>  
>  static void dwc3_gadget_interrupt(struct dwc3 *dwc,
> @@ -4718,7 +4736,15 @@ void dwc3_gadget_process_pending_events(struct dwc3 *dwc)
>  {
>  	if (dwc->pending_events) {
>  		dwc3_interrupt(dwc->irq_gadget, dwc->ev_buf);
> +		pm_runtime_put(dwc->dev);
>  		dwc->pending_events = false;
>  		enable_irq(dwc->irq_gadget);
> +		/*
> +		 * We have only stored the pending events as part
> +		 * of dwc3_interrupt() above, but those events are
> +		 * not yet handled. So explicitly invoke the
> +		 * interrupt handler for handling those events.
> +		 */
> +		dwc3_thread_interrupt(dwc->irq_gadget, dwc->ev_buf);

Why do we have to do this? If there are events, the threaded interrupt
should be woken up.

Thanks,
Thinh

>  	}
>  }
> -- 
> 2.17.1
> 

Re: [PATCH v3 2/3] dt-bindings: usb: snps,dwc3: Add allow-rtsusp-on-u3 property

[Elson Serrao]

On 7/11/2023 2:56 PM, Thinh Nguyen wrote:
> On Tue, Jul 11, 2023, Elson Roy Serrao wrote:
>> This property allows dwc3 runtime suspend when bus suspend interrupt
>> is received even with cable connected. This would allow the dwc3
>> controller to enter low power mode during bus suspend scenario.
>>
>> This property would particularly benefit dwc3 IPs where hibernation is
>> not enabled and the dwc3 low power mode entry/exit is handled by the
>> glue driver. The assumption here is that the platform using this dt
>> property is capable of detecting resume events to bring the controller
>> out of suspend.
>>
>> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
>> ---
>>   Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>> index a696f23730d3..18ad99a26dd9 100644
>> --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>> +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>> @@ -403,6 +403,11 @@ properties:
>>       description:
>>         Enable USB remote wakeup.
>>   
>> +  snps,allow-rtsusp-on-u3:
> 
> Please spell out the whole thing as "rtsusp" isn't clear. Also, it's not
> just for U3 right? For highspeed, it's L2.
> 
> How about the name that Roger use: "snps,gadget-keep-connect-sys-sleep"
> 
Done. Will make that modification and upload v4

>> +    description:
>> +      If True then dwc3 runtime suspend is allowed during bus suspend
>> +      case even with the USB cable connected.
>> +
>>   unevaluatedProperties: false
>>   
>>   required:
>> -- 
>> 2.17.1
>>
> 
> Did you Cc Rob, the devicetree maintainer?
> 

My bad. Thanks for pointing this out

Re: [PATCH v3 3/3] usb: dwc3: Modify runtime pm ops to handle bus suspend

[Elson Serrao]

On 7/11/2023 3:07 PM, Thinh Nguyen wrote:
> On Tue, Jul 11, 2023, Elson Roy Serrao wrote:
>> The current implementation blocks the runtime pm operations when cable
>> is connected. This would block platforms from entering system wide suspend
>> during bus suspend scenario. Modify the runtime pm ops to handle bus
>> suspend case for such platforms where the controller low power mode
>> entry/exit is handled by the glue driver. This enablement is controlled
>> through a dt property and platforms capable of detecting bus resume can
>> benefit from this feature. Also modify the remote wakeup operations to
>> trigger runtime resume before sending wakeup signal.
>>
>> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
>> ---
>>   drivers/usb/dwc3/core.c   | 26 ++++++++++++++++++++++---
>>   drivers/usb/dwc3/core.h   |  3 +++
>>   drivers/usb/dwc3/gadget.c | 40 ++++++++++++++++++++++++++++++++-------
>>   3 files changed, 59 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index f6689b731718..898c0f68e190 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -1534,6 +1534,9 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>>   	dwc->dis_split_quirk = device_property_read_bool(dev,
>>   				"snps,dis-split-quirk");
>>   
>> +	dwc->allow_rtsusp_on_u3 = device_property_read_bool(dev,
>> +				"snps,allow-rtsusp-on-u3");
>> +
>>   	dwc->lpm_nyet_threshold = lpm_nyet_threshold;
>>   	dwc->tx_de_emphasis = tx_de_emphasis;
>>   
>> @@ -1984,11 +1987,21 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
>>   {
>>   	unsigned long	flags;
>>   	u32 reg;
>> +	int link_state;
>>   
>>   	switch (dwc->current_dr_role) {
>>   	case DWC3_GCTL_PRTCAP_DEVICE:
>>   		if (pm_runtime_suspended(dwc->dev))
>>   			break;
>> +
>> +		if (dwc->connected) {
>> +			link_state = dwc3_gadget_get_link_state(dwc);
>> +			/* bus suspend case */
>> +			if (dwc->allow_rtsusp_on_u3 &&
>> +			    link_state == DWC3_LINK_STATE_U3)
>> +				break;
>> +			return -EBUSY;
>> +		}
>>   		dwc3_gadget_suspend(dwc);
>>   		synchronize_irq(dwc->irq_gadget);
>>   		dwc3_core_exit(dwc);
>> @@ -2045,6 +2058,9 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
>>   
>>   	switch (dwc->current_dr_role) {
>>   	case DWC3_GCTL_PRTCAP_DEVICE:
>> +		/* bus resume case */
>> +		if (dwc->connected)
>> +			break;
>>   		ret = dwc3_core_init_for_resume(dwc);
>>   		if (ret)
>>   			return ret;
>> @@ -2123,9 +2139,6 @@ static int dwc3_runtime_suspend(struct device *dev)
>>   	struct dwc3     *dwc = dev_get_drvdata(dev);
>>   	int		ret;
>>   
>> -	if (dwc3_runtime_checks(dwc))
>> -		return -EBUSY;
>> -
>>   	ret = dwc3_suspend_common(dwc, PMSG_AUTO_SUSPEND);
>>   	if (ret)
>>   		return ret;
>> @@ -2160,9 +2173,15 @@ static int dwc3_runtime_resume(struct device *dev)
>>   static int dwc3_runtime_idle(struct device *dev)
>>   {
>>   	struct dwc3     *dwc = dev_get_drvdata(dev);
>> +	int		link_state;
>>   
>>   	switch (dwc->current_dr_role) {
>>   	case DWC3_GCTL_PRTCAP_DEVICE:
>> +		link_state = dwc3_gadget_get_link_state(dwc);
>> +		/* for bus suspend case return success */
>> +		if (dwc->allow_rtsusp_on_u3 && dwc->connected &&
>> +		    link_state == DWC3_LINK_STATE_U3)
>> +			goto autosuspend;
>>   		if (dwc3_runtime_checks(dwc))
>>   			return -EBUSY;
>>   		break;
>> @@ -2172,6 +2191,7 @@ static int dwc3_runtime_idle(struct device *dev)
>>   		break;
>>   	}
>>   
>> +autosuspend:
>>   	pm_runtime_mark_last_busy(dev);
>>   	pm_runtime_autosuspend(dev);
>>   
>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>> index 8b1295e4dcdd..33b2ccbbd963 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -1127,6 +1127,8 @@ struct dwc3_scratchpad_array {
>>    * @num_ep_resized: carries the current number endpoints which have had its tx
>>    *		    fifo resized.
>>    * @debug_root: root debugfs directory for this device to put its files in.
>> + * @allow_rtsusp_on_u3: true if dwc3 runtime suspend is allowed during bus
>> + *			suspend scenario.
>>    */
>>   struct dwc3 {
>>   	struct work_struct	drd_work;
>> @@ -1343,6 +1345,7 @@ struct dwc3 {
>>   	int			last_fifo_depth;
>>   	int			num_ep_resized;
>>   	struct dentry		*debug_root;
>> +	bool			allow_rtsusp_on_u3;
>>   };
>>   
>>   #define INCRX_BURST_MODE 0
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 5fd067151fbf..0797cffa2d48 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -2401,15 +2401,21 @@ static int dwc3_gadget_wakeup(struct usb_gadget *g)
>>   		return -EINVAL;
>>   	}
>>   
>> -	spin_lock_irqsave(&dwc->lock, flags);
>>   	if (!dwc->gadget->wakeup_armed) {
>>   		dev_err(dwc->dev, "not armed for remote wakeup\n");
>> -		spin_unlock_irqrestore(&dwc->lock, flags);
>>   		return -EINVAL;
>>   	}
>> -	ret = __dwc3_gadget_wakeup(dwc, true);
>>   
>> +	ret = pm_runtime_resume_and_get(dwc->dev);
>> +	if (ret < 0) {
>> +		pm_runtime_set_suspended(dwc->dev);
>> +		return ret;
>> +	}
>> +
>> +	spin_lock_irqsave(&dwc->lock, flags);
>> +	ret = __dwc3_gadget_wakeup(dwc, true);
>>   	spin_unlock_irqrestore(&dwc->lock, flags);
>> +	pm_runtime_put_noidle(dwc->dev);
>>   
>>   	return ret;
>>   }
>> @@ -2428,6 +2434,12 @@ static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int intf_id)
>>   		return -EINVAL;
>>   	}
>>   
>> +	ret = pm_runtime_resume_and_get(dwc->dev);
>> +	if (ret < 0) {
>> +		pm_runtime_set_suspended(dwc->dev);
>> +		return ret;
>> +	}
>> +
>>   	spin_lock_irqsave(&dwc->lock, flags);
>>   	/*
>>   	 * If the link is in U3, signal for remote wakeup and wait for the
>> @@ -2438,6 +2450,7 @@ static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int intf_id)
>>   		ret = __dwc3_gadget_wakeup(dwc, false);
>>   		if (ret) {
>>   			spin_unlock_irqrestore(&dwc->lock, flags);
>> +			pm_runtime_put_noidle(dwc->dev);
>>   			return -EINVAL;
>>   		}
>>   		dwc3_resume_gadget(dwc);
>> @@ -2452,6 +2465,7 @@ static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int intf_id)
>>   		dev_err(dwc->dev, "function remote wakeup failed, ret:%d\n", ret);
>>   
>>   	spin_unlock_irqrestore(&dwc->lock, flags);
>> +	pm_runtime_put_noidle(dwc->dev);
>>   
>>   	return ret;
>>   }
>> @@ -2732,21 +2746,23 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
>>   	/*
>>   	 * Avoid issuing a runtime resume if the device is already in the
>>   	 * suspended state during gadget disconnect.  DWC3 gadget was already
>> -	 * halted/stopped during runtime suspend.
>> +	 * halted/stopped during runtime suspend except for bus suspend case
>> +	 * where we would have skipped the controller halt.
>>   	 */
>>   	if (!is_on) {
>>   		pm_runtime_barrier(dwc->dev);
>> -		if (pm_runtime_suspended(dwc->dev))
>> +		if (pm_runtime_suspended(dwc->dev) && !dwc->connected)
>>   			return 0;
>>   	}
>>   
>>   	/*
>>   	 * Check the return value for successful resume, or error.  For a
>>   	 * successful resume, the DWC3 runtime PM resume routine will handle
>> -	 * the run stop sequence, so avoid duplicate operations here.
>> +	 * the run stop sequence except for bus resume case, so avoid
>> +	 * duplicate operations here.
>>   	 */
>>   	ret = pm_runtime_get_sync(dwc->dev);
>> -	if (!ret || ret < 0) {
>> +	if ((!ret && !dwc->connected) || ret < 0) {
>>   		pm_runtime_put(dwc->dev);
>>   		if (ret < 0)
>>   			pm_runtime_set_suspended(dwc->dev);
>> @@ -4331,6 +4347,8 @@ static void dwc3_gadget_suspend_interrupt(struct dwc3 *dwc,
>>   	}
>>   
>>   	dwc->link_state = next;
>> +	pm_runtime_mark_last_busy(dwc->dev);
>> +	pm_request_autosuspend(dwc->dev);
>>   }
>>   
>>   static void dwc3_gadget_interrupt(struct dwc3 *dwc,
>> @@ -4718,7 +4736,15 @@ void dwc3_gadget_process_pending_events(struct dwc3 *dwc)
>>   {
>>   	if (dwc->pending_events) {
>>   		dwc3_interrupt(dwc->irq_gadget, dwc->ev_buf);
>> +		pm_runtime_put(dwc->dev);
>>   		dwc->pending_events = false;
>>   		enable_irq(dwc->irq_gadget);
>> +		/*
>> +		 * We have only stored the pending events as part
>> +		 * of dwc3_interrupt() above, but those events are
>> +		 * not yet handled. So explicitly invoke the
>> +		 * interrupt handler for handling those events.
>> +		 */
>> +		dwc3_thread_interrupt(dwc->irq_gadget, dwc->ev_buf);
> 
> Why do we have to do this? If there are events, the threaded interrupt
> should be woken up.
> 

dwc3_thread_interrupt will be woken up only if dwc3_interrupt() handler 
is invoked by the interrupt framework when the return value of 
IRQ_WAKE_THREAD is handled. But while processing the pending events the 
interrupt framework is not involved. We explicitly invoke the 
dwc3_interrupt() above within the dwc3 driver. So the 
dwc3_thread_interrupt() has to be explicitly invoked as well for 
processing those pending events.

Thanks
Elson

Re: [PATCH v3 3/3] usb: dwc3: Modify runtime pm ops to handle bus suspend

[Elson Serrao]

On 7/11/2023 3:51 PM, Elson Serrao wrote:
> 
> 
> On 7/11/2023 3:07 PM, Thinh Nguyen wrote:
>> On Tue, Jul 11, 2023, Elson Roy Serrao wrote:
>>> The current implementation blocks the runtime pm operations when cable
>>> is connected. This would block platforms from entering system wide 
>>> suspend
>>> during bus suspend scenario. Modify the runtime pm ops to handle bus
>>> suspend case for such platforms where the controller low power mode
>>> entry/exit is handled by the glue driver. This enablement is controlled
>>> through a dt property and platforms capable of detecting bus resume can
>>> benefit from this feature. Also modify the remote wakeup operations to
>>> trigger runtime resume before sending wakeup signal.
>>>
>>> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
>>> ---
>>>   drivers/usb/dwc3/core.c   | 26 ++++++++++++++++++++++---
>>>   drivers/usb/dwc3/core.h   |  3 +++
>>>   drivers/usb/dwc3/gadget.c | 40 ++++++++++++++++++++++++++++++++-------
>>>   3 files changed, 59 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>> index f6689b731718..898c0f68e190 100644
>>> --- a/drivers/usb/dwc3/core.c
>>> +++ b/drivers/usb/dwc3/core.c
>>> @@ -1534,6 +1534,9 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>>>       dwc->dis_split_quirk = device_property_read_bool(dev,
>>>                   "snps,dis-split-quirk");
>>> +    dwc->allow_rtsusp_on_u3 = device_property_read_bool(dev,
>>> +                "snps,allow-rtsusp-on-u3");
>>> +
>>>       dwc->lpm_nyet_threshold = lpm_nyet_threshold;
>>>       dwc->tx_de_emphasis = tx_de_emphasis;
>>> @@ -1984,11 +1987,21 @@ static int dwc3_suspend_common(struct dwc3 
>>> *dwc, pm_message_t msg)
>>>   {
>>>       unsigned long    flags;
>>>       u32 reg;
>>> +    int link_state;
>>>       switch (dwc->current_dr_role) {
>>>       case DWC3_GCTL_PRTCAP_DEVICE:
>>>           if (pm_runtime_suspended(dwc->dev))
>>>               break;
>>> +
>>> +        if (dwc->connected) {
>>> +            link_state = dwc3_gadget_get_link_state(dwc);
>>> +            /* bus suspend case */
>>> +            if (dwc->allow_rtsusp_on_u3 &&
>>> +                link_state == DWC3_LINK_STATE_U3)
>>> +                break;
>>> +            return -EBUSY;
>>> +        }
>>>           dwc3_gadget_suspend(dwc);
>>>           synchronize_irq(dwc->irq_gadget);
>>>           dwc3_core_exit(dwc);
>>> @@ -2045,6 +2058,9 @@ static int dwc3_resume_common(struct dwc3 *dwc, 
>>> pm_message_t msg)
>>>       switch (dwc->current_dr_role) {
>>>       case DWC3_GCTL_PRTCAP_DEVICE:
>>> +        /* bus resume case */
>>> +        if (dwc->connected)
>>> +            break;
>>>           ret = dwc3_core_init_for_resume(dwc);
>>>           if (ret)
>>>               return ret;
>>> @@ -2123,9 +2139,6 @@ static int dwc3_runtime_suspend(struct device 
>>> *dev)
>>>       struct dwc3     *dwc = dev_get_drvdata(dev);
>>>       int        ret;
>>> -    if (dwc3_runtime_checks(dwc))
>>> -        return -EBUSY;
>>> -
>>>       ret = dwc3_suspend_common(dwc, PMSG_AUTO_SUSPEND);
>>>       if (ret)
>>>           return ret;
>>> @@ -2160,9 +2173,15 @@ static int dwc3_runtime_resume(struct device 
>>> *dev)
>>>   static int dwc3_runtime_idle(struct device *dev)
>>>   {
>>>       struct dwc3     *dwc = dev_get_drvdata(dev);
>>> +    int        link_state;
>>>       switch (dwc->current_dr_role) {
>>>       case DWC3_GCTL_PRTCAP_DEVICE:
>>> +        link_state = dwc3_gadget_get_link_state(dwc);
>>> +        /* for bus suspend case return success */
>>> +        if (dwc->allow_rtsusp_on_u3 && dwc->connected &&
>>> +            link_state == DWC3_LINK_STATE_U3)
>>> +            goto autosuspend;
>>>           if (dwc3_runtime_checks(dwc))
>>>               return -EBUSY;
>>>           break;
>>> @@ -2172,6 +2191,7 @@ static int dwc3_runtime_idle(struct device *dev)
>>>           break;
>>>       }
>>> +autosuspend:
>>>       pm_runtime_mark_last_busy(dev);
>>>       pm_runtime_autosuspend(dev);
>>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>>> index 8b1295e4dcdd..33b2ccbbd963 100644
>>> --- a/drivers/usb/dwc3/core.h
>>> +++ b/drivers/usb/dwc3/core.h
>>> @@ -1127,6 +1127,8 @@ struct dwc3_scratchpad_array {
>>>    * @num_ep_resized: carries the current number endpoints which have 
>>> had its tx
>>>    *            fifo resized.
>>>    * @debug_root: root debugfs directory for this device to put its 
>>> files in.
>>> + * @allow_rtsusp_on_u3: true if dwc3 runtime suspend is allowed 
>>> during bus
>>> + *            suspend scenario.
>>>    */
>>>   struct dwc3 {
>>>       struct work_struct    drd_work;
>>> @@ -1343,6 +1345,7 @@ struct dwc3 {
>>>       int            last_fifo_depth;
>>>       int            num_ep_resized;
>>>       struct dentry        *debug_root;
>>> +    bool            allow_rtsusp_on_u3;
>>>   };
>>>   #define INCRX_BURST_MODE 0
>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>> index 5fd067151fbf..0797cffa2d48 100644
>>> --- a/drivers/usb/dwc3/gadget.c
>>> +++ b/drivers/usb/dwc3/gadget.c
>>> @@ -2401,15 +2401,21 @@ static int dwc3_gadget_wakeup(struct 
>>> usb_gadget *g)
>>>           return -EINVAL;
>>>       }
>>> -    spin_lock_irqsave(&dwc->lock, flags);
>>>       if (!dwc->gadget->wakeup_armed) {
>>>           dev_err(dwc->dev, "not armed for remote wakeup\n");
>>> -        spin_unlock_irqrestore(&dwc->lock, flags);
>>>           return -EINVAL;
>>>       }
>>> -    ret = __dwc3_gadget_wakeup(dwc, true);
>>> +    ret = pm_runtime_resume_and_get(dwc->dev);
>>> +    if (ret < 0) {
>>> +        pm_runtime_set_suspended(dwc->dev);
>>> +        return ret;
>>> +    }
>>> +
>>> +    spin_lock_irqsave(&dwc->lock, flags);
>>> +    ret = __dwc3_gadget_wakeup(dwc, true);
>>>       spin_unlock_irqrestore(&dwc->lock, flags);
>>> +    pm_runtime_put_noidle(dwc->dev);
>>>       return ret;
>>>   }
>>> @@ -2428,6 +2434,12 @@ static int dwc3_gadget_func_wakeup(struct 
>>> usb_gadget *g, int intf_id)
>>>           return -EINVAL;
>>>       }
>>> +    ret = pm_runtime_resume_and_get(dwc->dev);
>>> +    if (ret < 0) {
>>> +        pm_runtime_set_suspended(dwc->dev);
>>> +        return ret;
>>> +    }
>>> +
>>>       spin_lock_irqsave(&dwc->lock, flags);
>>>       /*
>>>        * If the link is in U3, signal for remote wakeup and wait for the
>>> @@ -2438,6 +2450,7 @@ static int dwc3_gadget_func_wakeup(struct 
>>> usb_gadget *g, int intf_id)
>>>           ret = __dwc3_gadget_wakeup(dwc, false);
>>>           if (ret) {
>>>               spin_unlock_irqrestore(&dwc->lock, flags);
>>> +            pm_runtime_put_noidle(dwc->dev);
>>>               return -EINVAL;
>>>           }
>>>           dwc3_resume_gadget(dwc);
>>> @@ -2452,6 +2465,7 @@ static int dwc3_gadget_func_wakeup(struct 
>>> usb_gadget *g, int intf_id)
>>>           dev_err(dwc->dev, "function remote wakeup failed, 
>>> ret:%d\n", ret);
>>>       spin_unlock_irqrestore(&dwc->lock, flags);
>>> +    pm_runtime_put_noidle(dwc->dev);
>>>       return ret;
>>>   }
>>> @@ -2732,21 +2746,23 @@ static int dwc3_gadget_pullup(struct 
>>> usb_gadget *g, int is_on)
>>>       /*
>>>        * Avoid issuing a runtime resume if the device is already in the
>>>        * suspended state during gadget disconnect.  DWC3 gadget was 
>>> already
>>> -     * halted/stopped during runtime suspend.
>>> +     * halted/stopped during runtime suspend except for bus suspend 
>>> case
>>> +     * where we would have skipped the controller halt.
>>>        */
>>>       if (!is_on) {
>>>           pm_runtime_barrier(dwc->dev);
>>> -        if (pm_runtime_suspended(dwc->dev))
>>> +        if (pm_runtime_suspended(dwc->dev) && !dwc->connected)
>>>               return 0;
>>>       }
>>>       /*
>>>        * Check the return value for successful resume, or error.  For a
>>>        * successful resume, the DWC3 runtime PM resume routine will 
>>> handle
>>> -     * the run stop sequence, so avoid duplicate operations here.
>>> +     * the run stop sequence except for bus resume case, so avoid
>>> +     * duplicate operations here.
>>>        */
>>>       ret = pm_runtime_get_sync(dwc->dev);
>>> -    if (!ret || ret < 0) {
>>> +    if ((!ret && !dwc->connected) || ret < 0) {
>>>           pm_runtime_put(dwc->dev);
>>>           if (ret < 0)
>>>               pm_runtime_set_suspended(dwc->dev);
>>> @@ -4331,6 +4347,8 @@ static void 
>>> dwc3_gadget_suspend_interrupt(struct dwc3 *dwc,
>>>       }
>>>       dwc->link_state = next;
>>> +    pm_runtime_mark_last_busy(dwc->dev);
>>> +    pm_request_autosuspend(dwc->dev);
>>>   }
>>>   static void dwc3_gadget_interrupt(struct dwc3 *dwc,
>>> @@ -4718,7 +4736,15 @@ void dwc3_gadget_process_pending_events(struct 
>>> dwc3 *dwc)
>>>   {
>>>       if (dwc->pending_events) {
>>>           dwc3_interrupt(dwc->irq_gadget, dwc->ev_buf);
>>> +        pm_runtime_put(dwc->dev);
>>>           dwc->pending_events = false;
>>>           enable_irq(dwc->irq_gadget);
>>> +        /*
>>> +         * We have only stored the pending events as part
>>> +         * of dwc3_interrupt() above, but those events are
>>> +         * not yet handled. So explicitly invoke the
>>> +         * interrupt handler for handling those events.
>>> +         */
>>> +        dwc3_thread_interrupt(dwc->irq_gadget, dwc->ev_buf);
>>
>> Why do we have to do this? If there are events, the threaded interrupt
>> should be woken up.
>>
> 
> dwc3_thread_interrupt will be woken up only if dwc3_interrupt() handler 
> is invoked by the interrupt framework when the return value of 
> IRQ_WAKE_THREAD is handled. But while processing the pending events the 
> interrupt framework is not involved. We explicitly invoke the 
> dwc3_interrupt() above within the dwc3 driver. So the 
> dwc3_thread_interrupt() has to be explicitly invoked as well for 
> processing those pending events.
> 

Perhaps we can make it more optimal by checking the return value like below?

void dwc3_gadget_process_pending_events(struct dwc3 *dwc)
{
         irqreturn_t ret;

         if (dwc->pending_events) {
                 ret = dwc3_interrupt(dwc->irq_gadget, dwc->ev_buf);
                 pm_runtime_put(dwc->dev);
                 dwc->pending_events = false;
                 enable_irq(dwc->irq_gadget);
                 /*
                  * We have only stored the pending events as part
                  * of dwc3_interrupt() above, but those events are
                  * not yet handled. So explicitly invoke the
                  * interrupt handler for handling those events.
                  */
                 if (ret == IRQ_WAKE_THREAD)
                         dwc3_thread_interrupt(dwc->irq_gadget, 
dwc->ev_buf);
         }
}

Re: [PATCH v3 2/3] dt-bindings: usb: snps,dwc3: Add allow-rtsusp-on-u3 property

[Krzysztof Kozlowski]

On 11/07/2023 19:43, Elson Roy Serrao wrote:
> This property allows dwc3 runtime suspend when bus suspend interrupt
> is received even with cable connected. This would allow the dwc3
> controller to enter low power mode during bus suspend scenario.
> 
> This property would particularly benefit dwc3 IPs where hibernation is
> not enabled and the dwc3 low power mode entry/exit is handled by the
> glue driver. The assumption here is that the platform using this dt
> property is capable of detecting resume events to bring the controller
> out of suspend.

v3 and still ignoring maintainers?

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC. It might happen, that command when run on an older
kernel, gives you outdated entries. Therefore please be sure you base
your patches on recent Linux kernel.

You missed at least DT list (maybe more), so this won't be tested by
automated tooling. Performing review on untested code might be a waste
of time, thus I will skip this patch entirely till you follow the
process allowing the patch to be tested.

Please kindly resend and include all necessary To/Cc entries.


Best regards,
Krzysztof

Re: [PATCH v3 2/3] dt-bindings: usb: snps,dwc3: Add allow-rtsusp-on-u3 property

[Roger Quadros]

+Rob

On 12/07/2023 00:56, Thinh Nguyen wrote:
> On Tue, Jul 11, 2023, Elson Roy Serrao wrote:
>> This property allows dwc3 runtime suspend when bus suspend interrupt
>> is received even with cable connected. This would allow the dwc3
>> controller to enter low power mode during bus suspend scenario.
>>
>> This property would particularly benefit dwc3 IPs where hibernation is
>> not enabled and the dwc3 low power mode entry/exit is handled by the
>> glue driver. The assumption here is that the platform using this dt
>> property is capable of detecting resume events to bring the controller
>> out of suspend.
>>
>> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
>> ---
>>  Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>> index a696f23730d3..18ad99a26dd9 100644
>> --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>> +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>> @@ -403,6 +403,11 @@ properties:
>>      description:
>>        Enable USB remote wakeup.
>>  
>> +  snps,allow-rtsusp-on-u3:
> 
> Please spell out the whole thing as "rtsusp" isn't clear. Also, it's not
> just for U3 right? For highspeed, it's L2.
> 
> How about the name that Roger use: "snps,gadget-keep-connect-sys-sleep"

That property was meant to keep the USB device controller connected
during system sleep. So that name may not be appropriate here as this
is about allowing controller runtime suspend during USB suspend.

> 
>> +    description:
>> +      If True then dwc3 runtime suspend is allowed during bus suspend
>> +      case even with the USB cable connected.
>> +
>>  unevaluatedProperties: false
>>  
>>  required:
>> -- 
>> 2.17.1
>>
> 
> Did you Cc Rob, the devicetree maintainer?
> 
> Thanks,
> Thinh

-- 
cheers,
-roger

Re: [PATCH v3 0/3] Support dwc3 runtime suspend during bus suspend

[Roger Quadros]

Hi Elson,

On 11/07/2023 20:43, Elson Roy Serrao wrote:
> Changes in v3
>  - Added a dt property 'snps,allow-rtsusp-on-u3' to make this feature platform
>    dependent as per the feedback from Thinh N.
>  - Changed the RT idle/suspend/resume handling to device mode specific and dt
>    property dependent.
>  - Modified the cover letter to document how resume is handled on qcom platforms.
>  
> Changes in v2
>  - Used pm_runtime_resume_and_get() API instead of pm_runtime_get_sync()
>    as suggested by Dan.
>  - Handled the return value in ether_wakeup_host to print error message.
> 
> When a USB link is idle, the host sends a bus suspend event to the device
> so that the device can save power. But true power savings during bus
> suspend can be seen only if we let the USB controller enter low power
> mode and turn off the clocks. Vendor drivers may have their own runtime
> power management framework to power up/down the controller. But since
> vendor drivers' runtime suspend/resume routines depend on the dwc3 child
> node we would need a framework to trigger dwc3 runtime pm ops whenever a
> bus suspend is received. If the device wants to exit from bus suspend
> state it can send a wakeup signal to the host by first bringing out the
> controller from low power mode. This series implements the needed
> framework to achieve this functionality when a bus suspend interupt is
> received. The assumption here is that the dwc3 hibernation feature is not
> enabled and the platform is responsible in detecting the resume events to
> bring the controller out of suspend.
> 
> On Qualcomm platforms the bus resume is handled through Phy and informed to
> software through wakeup capable phy interrupts.
> usb2 PHY is configured to detect the Resume K event and sends an interrupt
> when this event is detected. This would trigger the runtime resume of the
> glue driver which would intrinsically wakeup the dwc3 child. In case of usb3 PHY,
> it is configured to detect the LFPS wake signal during bus resume and the
> corresponding interrupt triggers the runtime resume of the glue driver.

Subject says runtime suspend. But are you testing system sleep/wakeup as well
while USB is suspended?

> 
> The series is organized in below fashion:
> Patch 1: This includes the modification needed from function drivers to let
> UDC enter low power mode with u_ether as an example.
> Patch 2: New dt property to allow dwc3 runtime suspedn during bus suspend scenario. 
> Patch 3: This has the modification needed in the UDC driver to trigger runtime
> suspend whene a bus suspend interrupt is received. Since this is a platform
> dependent change it is made applicable through a dt property. This also handles
> resume and remote wakeup modifications from power management perspective.
> 
> Elson Roy Serrao (3):
>   usb: function: u_ether: Handle rx requests during suspend/resume
>   dt-bindings: usb: snps,dwc3: Add allow-rtsusp-on-u3 property
>   usb: dwc3: Modify runtime pm ops to handle bus suspend
> 
>  .../devicetree/bindings/usb/snps,dwc3.yaml    |  5 ++
>  drivers/usb/dwc3/core.c                       | 26 ++++++++--
>  drivers/usb/dwc3/core.h                       |  3 ++
>  drivers/usb/dwc3/gadget.c                     | 40 +++++++++++++---
>  drivers/usb/gadget/function/u_ether.c         | 47 +++++++++++++++----
>  5 files changed, 102 insertions(+), 19 deletions(-)
> 

-- 
cheers,
-roger

Re: [PATCH v3 3/3] usb: dwc3: Modify runtime pm ops to handle bus suspend

[Roger Quadros]

On 11/07/2023 20:43, Elson Roy Serrao wrote:
> The current implementation blocks the runtime pm operations when cable
> is connected. This would block platforms from entering system wide suspend
> during bus suspend scenario. Modify the runtime pm ops to handle bus
> suspend case for such platforms where the controller low power mode
> entry/exit is handled by the glue driver. This enablement is controlled
> through a dt property and platforms capable of detecting bus resume can
> benefit from this feature. Also modify the remote wakeup operations to
> trigger runtime resume before sending wakeup signal.
> 
> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
> ---
>  drivers/usb/dwc3/core.c   | 26 ++++++++++++++++++++++---
>  drivers/usb/dwc3/core.h   |  3 +++
>  drivers/usb/dwc3/gadget.c | 40 ++++++++++++++++++++++++++++++++-------
>  3 files changed, 59 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index f6689b731718..898c0f68e190 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1534,6 +1534,9 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>  	dwc->dis_split_quirk = device_property_read_bool(dev,
>  				"snps,dis-split-quirk");
>  
> +	dwc->allow_rtsusp_on_u3 = device_property_read_bool(dev,
> +				"snps,allow-rtsusp-on-u3");
> +
>  	dwc->lpm_nyet_threshold = lpm_nyet_threshold;
>  	dwc->tx_de_emphasis = tx_de_emphasis;
>  
> @@ -1984,11 +1987,21 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
>  {
>  	unsigned long	flags;
>  	u32 reg;
> +	int link_state;
>  
>  	switch (dwc->current_dr_role) {
>  	case DWC3_GCTL_PRTCAP_DEVICE:
>  		if (pm_runtime_suspended(dwc->dev))
>  			break;
> +
> +		if (dwc->connected) {
> +			link_state = dwc3_gadget_get_link_state(dwc);
> +			/* bus suspend case */
> +			if (dwc->allow_rtsusp_on_u3 &&
> +			    link_state == DWC3_LINK_STATE_U3> +				break;

dwc3_suspend_common() is called both during runtime suspend and system sleep with
appropriate pm_msg_t argument.
Do you want the same behavior for both cases?

You are not checking if the gadget driver was put into suspend state or not.
So, instead of just checking line state, should we also check that we processed
the suspend interrupt and gadget driver has been suspended as well?
i.e. add a SW flag dwc3->suspended and set it at end of dwc3_suspend_gadget()
and clear it at beginning of dwc3_resume_gadget().
Check for that flag in the above if condition.
i.e. if (dwc->connected && dwc->suspended)

> +			return -EBUSY;
> +		}
>  		dwc3_gadget_suspend(dwc);
>  		synchronize_irq(dwc->irq_gadget);
>  		dwc3_core_exit(dwc);
> @@ -2045,6 +2058,9 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
>  
>  	switch (dwc->current_dr_role) {
>  	case DWC3_GCTL_PRTCAP_DEVICE:
> +		/* bus resume case */
> +		if (dwc->connected)

dwc->connected might get cleard to false if device was disconnected. So this is not reliable.
You might want to set another SW flag to track this special case of suspend
with controller kept active.

> +			break;
>  		ret = dwc3_core_init_for_resume(dwc);
>  		if (ret)
>  			return ret;
> @@ -2123,9 +2139,6 @@ static int dwc3_runtime_suspend(struct device *dev)
>  	struct dwc3     *dwc = dev_get_drvdata(dev);
>  	int		ret;
>  
> -	if (dwc3_runtime_checks(dwc))
> -		return -EBUSY;
> -
Instead of removing this how about modifying dwc3_runtime_checks(). see below.

>  	ret = dwc3_suspend_common(dwc, PMSG_AUTO_SUSPEND);
>  	if (ret)
>  		return ret;
> @@ -2160,9 +2173,15 @@ static int dwc3_runtime_resume(struct device *dev)
>  static int dwc3_runtime_idle(struct device *dev)
>  {
>  	struct dwc3     *dwc = dev_get_drvdata(dev);
> +	int		link_state;
>  
>  	switch (dwc->current_dr_role) {
>  	case DWC3_GCTL_PRTCAP_DEVICE:
> +		link_state = dwc3_gadget_get_link_state(dwc);
> +		/* for bus suspend case return success */
> +		if (dwc->allow_rtsusp_on_u3 && dwc->connected &&
> +		    link_state == DWC3_LINK_STATE_U3)
> +			goto autosuspend;

why not just break?

>  		if (dwc3_runtime_checks(dwc))

what about this dwc3_runtime_checks()?
Looks like the if condition you added above can go in dwc3_runtime_checks() instead.

>  			return -EBUSY;
>  		break;
> @@ -2172,6 +2191,7 @@ static int dwc3_runtime_idle(struct device *dev)
>  		break;
>  	}
>  
> +autosuspend:
>  	pm_runtime_mark_last_busy(dev);
>  	pm_runtime_autosuspend(dev);
>  
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 8b1295e4dcdd..33b2ccbbd963 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -1127,6 +1127,8 @@ struct dwc3_scratchpad_array {
>   * @num_ep_resized: carries the current number endpoints which have had its tx
>   *		    fifo resized.
>   * @debug_root: root debugfs directory for this device to put its files in.
> + * @allow_rtsusp_on_u3: true if dwc3 runtime suspend is allowed during bus
> + *			suspend scenario.

what about system sleep?

>   */
>  struct dwc3 {
>  	struct work_struct	drd_work;
> @@ -1343,6 +1345,7 @@ struct dwc3 {
>  	int			last_fifo_depth;
>  	int			num_ep_resized;
>  	struct dentry		*debug_root;
> +	bool			allow_rtsusp_on_u3;
>  };
>  
>  #define INCRX_BURST_MODE 0
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 5fd067151fbf..0797cffa2d48 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2401,15 +2401,21 @@ static int dwc3_gadget_wakeup(struct usb_gadget *g)
>  		return -EINVAL;
>  	}
>  
> -	spin_lock_irqsave(&dwc->lock, flags);
>  	if (!dwc->gadget->wakeup_armed) {
>  		dev_err(dwc->dev, "not armed for remote wakeup\n");
> -		spin_unlock_irqrestore(&dwc->lock, flags);
>  		return -EINVAL;
>  	}
> -	ret = __dwc3_gadget_wakeup(dwc, true);
>  
> +	ret = pm_runtime_resume_and_get(dwc->dev);
> +	if (ret < 0) {
> +		pm_runtime_set_suspended(dwc->dev);
> +		return ret;
> +	}
> +
> +	spin_lock_irqsave(&dwc->lock, flags);
> +	ret = __dwc3_gadget_wakeup(dwc, true);
>  	spin_unlock_irqrestore(&dwc->lock, flags);
> +	pm_runtime_put_noidle(dwc->dev);
>  
>  	return ret;
>  }
> @@ -2428,6 +2434,12 @@ static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int intf_id)
>  		return -EINVAL;
>  	}
>  
> +	ret = pm_runtime_resume_and_get(dwc->dev);
> +	if (ret < 0) {
> +		pm_runtime_set_suspended(dwc->dev);
> +		return ret;
> +	}
> +
>  	spin_lock_irqsave(&dwc->lock, flags);
>  	/*
>  	 * If the link is in U3, signal for remote wakeup and wait for the
> @@ -2438,6 +2450,7 @@ static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int intf_id)
>  		ret = __dwc3_gadget_wakeup(dwc, false);
>  		if (ret) {
>  			spin_unlock_irqrestore(&dwc->lock, flags);
> +			pm_runtime_put_noidle(dwc->dev);
>  			return -EINVAL;
>  		}
>  		dwc3_resume_gadget(dwc);
> @@ -2452,6 +2465,7 @@ static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int intf_id)
>  		dev_err(dwc->dev, "function remote wakeup failed, ret:%d\n", ret);
>  
>  	spin_unlock_irqrestore(&dwc->lock, flags);
> +	pm_runtime_put_noidle(dwc->dev);
>  
>  	return ret;
>  }
> @@ -2732,21 +2746,23 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
>  	/*
>  	 * Avoid issuing a runtime resume if the device is already in the
>  	 * suspended state during gadget disconnect.  DWC3 gadget was already
> -	 * halted/stopped during runtime suspend.
> +	 * halted/stopped during runtime suspend except for bus suspend case
> +	 * where we would have skipped the controller halt.
>  	 */
>  	if (!is_on) {
>  		pm_runtime_barrier(dwc->dev);
> -		if (pm_runtime_suspended(dwc->dev))
> +		if (pm_runtime_suspended(dwc->dev) && !dwc->connected)

dwc->connected will to to false on disconnect. I'm not sure if we can rely
on this flag here.

>  			return 0;
>  	}
>  
>  	/*
>  	 * Check the return value for successful resume, or error.  For a
>  	 * successful resume, the DWC3 runtime PM resume routine will handle
> -	 * the run stop sequence, so avoid duplicate operations here.
> +	 * the run stop sequence except for bus resume case, so avoid
> +	 * duplicate operations here.
>  	 */
>  	ret = pm_runtime_get_sync(dwc->dev);
> -	if (!ret || ret < 0) {
> +	if ((!ret && !dwc->connected) || ret < 0) {

Why this check?

>  		pm_runtime_put(dwc->dev);
>  		if (ret < 0)
>  			pm_runtime_set_suspended(dwc->dev);
> @@ -4331,6 +4347,8 @@ static void dwc3_gadget_suspend_interrupt(struct dwc3 *dwc,
>  	}
>  
>  	dwc->link_state = next;
> +	pm_runtime_mark_last_busy(dwc->dev);
> +	pm_request_autosuspend(dwc->dev);

Not here. You should do this in dwc3_suspend_gadget() after gadget driver
has completed suspend.

>  }
>  
>  static void dwc3_gadget_interrupt(struct dwc3 *dwc,
> @@ -4718,7 +4736,15 @@ void dwc3_gadget_process_pending_events(struct dwc3 *dwc)
>  {
>  	if (dwc->pending_events) {
>  		dwc3_interrupt(dwc->irq_gadget, dwc->ev_buf);
> +		pm_runtime_put(dwc->dev);

Why the put here?

>  		dwc->pending_events = false;
>  		enable_irq(dwc->irq_gadget);
> +		/*
> +		 * We have only stored the pending events as part
> +		 * of dwc3_interrupt() above, but those events are
> +		 * not yet handled. So explicitly invoke the
> +		 * interrupt handler for handling those events.
> +		 */
> +		dwc3_thread_interrupt(dwc->irq_gadget, dwc->ev_buf);
>  	}
>  }

-- 
cheers,
-roger

Re: [PATCH v3 2/3] dt-bindings: usb: snps,dwc3: Add allow-rtsusp-on-u3 property

[Thinh Nguyen]

On Wed, Jul 12, 2023, Roger Quadros wrote:
> +Rob
> 
> On 12/07/2023 00:56, Thinh Nguyen wrote:
> > On Tue, Jul 11, 2023, Elson Roy Serrao wrote:
> >> This property allows dwc3 runtime suspend when bus suspend interrupt
> >> is received even with cable connected. This would allow the dwc3
> >> controller to enter low power mode during bus suspend scenario.
> >>
> >> This property would particularly benefit dwc3 IPs where hibernation is
> >> not enabled and the dwc3 low power mode entry/exit is handled by the
> >> glue driver. The assumption here is that the platform using this dt
> >> property is capable of detecting resume events to bring the controller
> >> out of suspend.
> >>
> >> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
> >> ---
> >>  Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 5 +++++
> >>  1 file changed, 5 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> >> index a696f23730d3..18ad99a26dd9 100644
> >> --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> >> +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> >> @@ -403,6 +403,11 @@ properties:
> >>      description:
> >>        Enable USB remote wakeup.
> >>  
> >> +  snps,allow-rtsusp-on-u3:
> > 
> > Please spell out the whole thing as "rtsusp" isn't clear. Also, it's not
> > just for U3 right? For highspeed, it's L2.
> > 
> > How about the name that Roger use: "snps,gadget-keep-connect-sys-sleep"
> 
> That property was meant to keep the USB device controller connected
> during system sleep. So that name may not be appropriate here as this
> is about allowing controller runtime suspend during USB suspend.
> 

Ah... you're right. Don't know what I was thinking. They're not the
same, and they may need to be separated. I'm not great with the naming,
but it can be something like this: "snps,runtime-suspend-on-usb-suspend"

BTW, I assume that your platform (both from Roger and Elson) does not
turn off the bus that handle DMA of events on suspend? (e.g. AXI bus)
Looking at the change, it looks like the events are still written in the
event buffer.

Thanks,
Thinh

Re: [PATCH v3 3/3] usb: dwc3: Modify runtime pm ops to handle bus suspend

[Elson Serrao]

On 7/12/2023 6:46 AM, Roger Quadros wrote:
> 
> 
> On 11/07/2023 20:43, Elson Roy Serrao wrote:
>> The current implementation blocks the runtime pm operations when cable
>> is connected. This would block platforms from entering system wide suspend
>> during bus suspend scenario. Modify the runtime pm ops to handle bus
>> suspend case for such platforms where the controller low power mode
>> entry/exit is handled by the glue driver. This enablement is controlled
>> through a dt property and platforms capable of detecting bus resume can
>> benefit from this feature. Also modify the remote wakeup operations to
>> trigger runtime resume before sending wakeup signal.
>>
>> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
>> ---
>>   drivers/usb/dwc3/core.c   | 26 ++++++++++++++++++++++---
>>   drivers/usb/dwc3/core.h   |  3 +++
>>   drivers/usb/dwc3/gadget.c | 40 ++++++++++++++++++++++++++++++++-------
>>   3 files changed, 59 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index f6689b731718..898c0f68e190 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -1534,6 +1534,9 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>>   	dwc->dis_split_quirk = device_property_read_bool(dev,
>>   				"snps,dis-split-quirk");
>>   
>> +	dwc->allow_rtsusp_on_u3 = device_property_read_bool(dev,
>> +				"snps,allow-rtsusp-on-u3");
>> +
>>   	dwc->lpm_nyet_threshold = lpm_nyet_threshold;
>>   	dwc->tx_de_emphasis = tx_de_emphasis;
>>   
>> @@ -1984,11 +1987,21 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
>>   {
>>   	unsigned long	flags;
>>   	u32 reg;
>> +	int link_state;
>>   
>>   	switch (dwc->current_dr_role) {
>>   	case DWC3_GCTL_PRTCAP_DEVICE:
>>   		if (pm_runtime_suspended(dwc->dev))
>>   			break;
>> +
>> +		if (dwc->connected) {
>> +			link_state = dwc3_gadget_get_link_state(dwc);
>> +			/* bus suspend case */
>> +			if (dwc->allow_rtsusp_on_u3 &&
>> +			    link_state == DWC3_LINK_STATE_U3> +				break;
> 
> dwc3_suspend_common() is called both during runtime suspend and system sleep with
> appropriate pm_msg_t argument.
> Do you want the same behavior for both cases?
> 
Thank you for your comments and feedback.
The goal here is to trigger runtime suspend when bus suspend interrupt 
is received. If dwc3 runtime suspend is executed, then for system sleep 
dwc3_suspend_common() is a no-op as we have a 
pm_runtime_suspended(dwc->dev) check in place in the above code.
The exception I can think of with these changes in place is below sequence
USB bus suspended---->Runtime suspend fails OR is not invoked for some 
reason--->System sleep is triggered.

We can extend this change to above case as well. I believe that was the 
intention of your patch (i.e avoid gadget disconnect if usb bus is 
suspended for system sleep case)
https://lore.kernel.org/linux-usb/20230320093447.32105-3-rogerq@kernel.org/

Please let me know your opinion on extending this series to system sleep 
also.


> You are not checking if the gadget driver was put into suspend state or not.
> So, instead of just checking line state, should we also check that we processed
> the suspend interrupt and gadget driver has been suspended as well?
> i.e. add a SW flag dwc3->suspended and set it at end of dwc3_suspend_gadget()
> and clear it at beginning of dwc3_resume_gadget().
> Check for that flag in the above if condition.
> i.e. if (dwc->connected && dwc->suspended)
>

We already have flags for these purposes (dwc->link_state and 
dwc->suspended) that we set when suspend interrupt is received and clear 
during wakeup. We can use these flags here

>> +			return -EBUSY;
>> +		}
>>   		dwc3_gadget_suspend(dwc);
>>   		synchronize_irq(dwc->irq_gadget);
>>   		dwc3_core_exit(dwc);
>> @@ -2045,6 +2058,9 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
>>   
>>   	switch (dwc->current_dr_role) {
>>   	case DWC3_GCTL_PRTCAP_DEVICE:
>> +		/* bus resume case */
>> +		if (dwc->connected)
> 
> dwc->connected might get cleard to false if device was disconnected. So this is not reliable.
> You might want to set another SW flag to track this special case of suspend
> with controller kept active.
> 

Are you referring to the DISCONNECT interrupt ? We dont handle gadget 
interrupts when in runtime suspended state and dwc3 driver has to be 
resumed first. The mechanism for this already exists in 
dwc3_check_event_buf() like below

	if (pm_runtime_suspended(dwc->dev)) {
		pm_runtime_get(dwc->dev);
		disable_irq_nosync(dwc->irq_gadget);
		dwc->pending_events = true;
		return IRQ_HANDLED;
	}

I cant think of any scenario where dwc->connected is cleared before 
calling resume_common() for bus suspend case. Please let me know if I am 
missing any corner case.

>> +			break;
>>   		ret = dwc3_core_init_for_resume(dwc);
>>   		if (ret)
>>   			return ret;
>> @@ -2123,9 +2139,6 @@ static int dwc3_runtime_suspend(struct device *dev)
>>   	struct dwc3     *dwc = dev_get_drvdata(dev);
>>   	int		ret;
>>   
>> -	if (dwc3_runtime_checks(dwc))
>> -		return -EBUSY;
>> -
> Instead of removing this how about modifying dwc3_runtime_checks(). see below.
>

Yes we can do that. But if we are planning to extend these changes to 
system sleep better to leave it here IMO.

>>   	ret = dwc3_suspend_common(dwc, PMSG_AUTO_SUSPEND);
>>   	if (ret)
>>   		return ret;
>> @@ -2160,9 +2173,15 @@ static int dwc3_runtime_resume(struct device *dev)
>>   static int dwc3_runtime_idle(struct device *dev)
>>   {
>>   	struct dwc3     *dwc = dev_get_drvdata(dev);
>> +	int		link_state;
>>   
>>   	switch (dwc->current_dr_role) {
>>   	case DWC3_GCTL_PRTCAP_DEVICE:
>> +		link_state = dwc3_gadget_get_link_state(dwc);
>> +		/* for bus suspend case return success */
>> +		if (dwc->allow_rtsusp_on_u3 && dwc->connected &&
>> +		    link_state == DWC3_LINK_STATE_U3)
>> +			goto autosuspend;
> 
> why not just break?
Done
> 
>>   		if (dwc3_runtime_checks(dwc))
> 
> what about this dwc3_runtime_checks()?
> Looks like the if condition you added above can go in dwc3_runtime_checks() instead.
> 
>>   			return -EBUSY;
>>   		break;
>> @@ -2172,6 +2191,7 @@ static int dwc3_runtime_idle(struct device *dev)
>>   		break;
>>   	}
>>   
>> +autosuspend:
>>   	pm_runtime_mark_last_busy(dev);
>>   	pm_runtime_autosuspend(dev);
>>   
>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>> index 8b1295e4dcdd..33b2ccbbd963 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -1127,6 +1127,8 @@ struct dwc3_scratchpad_array {
>>    * @num_ep_resized: carries the current number endpoints which have had its tx
>>    *		    fifo resized.
>>    * @debug_root: root debugfs directory for this device to put its files in.
>> + * @allow_rtsusp_on_u3: true if dwc3 runtime suspend is allowed during bus
>> + *			suspend scenario.
> 
> what about system sleep?
> 
Currently this series is tested for runtime suspend during bus suspend 
scenario. We can extend to system sleep like explained above.

>>    */
>>   struct dwc3 {
>>   	struct work_struct	drd_work;
>> @@ -1343,6 +1345,7 @@ struct dwc3 {
>>   	int			last_fifo_depth;
>>   	int			num_ep_resized;
>>   	struct dentry		*debug_root;
>> +	bool			allow_rtsusp_on_u3;
>>   };
>>   
>>   #define INCRX_BURST_MODE 0
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 5fd067151fbf..0797cffa2d48 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -2401,15 +2401,21 @@ static int dwc3_gadget_wakeup(struct usb_gadget *g)
>>   		return -EINVAL;
>>   	}
>>   
>> -	spin_lock_irqsave(&dwc->lock, flags);
>>   	if (!dwc->gadget->wakeup_armed) {
>>   		dev_err(dwc->dev, "not armed for remote wakeup\n");
>> -		spin_unlock_irqrestore(&dwc->lock, flags);
>>   		return -EINVAL;
>>   	}
>> -	ret = __dwc3_gadget_wakeup(dwc, true);
>>   
>> +	ret = pm_runtime_resume_and_get(dwc->dev);
>> +	if (ret < 0) {
>> +		pm_runtime_set_suspended(dwc->dev);
>> +		return ret;
>> +	}
>> +
>> +	spin_lock_irqsave(&dwc->lock, flags);
>> +	ret = __dwc3_gadget_wakeup(dwc, true);
>>   	spin_unlock_irqrestore(&dwc->lock, flags);
>> +	pm_runtime_put_noidle(dwc->dev);
>>   
>>   	return ret;
>>   }
>> @@ -2428,6 +2434,12 @@ static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int intf_id)
>>   		return -EINVAL;
>>   	}
>>   
>> +	ret = pm_runtime_resume_and_get(dwc->dev);
>> +	if (ret < 0) {
>> +		pm_runtime_set_suspended(dwc->dev);
>> +		return ret;
>> +	}
>> +
>>   	spin_lock_irqsave(&dwc->lock, flags);
>>   	/*
>>   	 * If the link is in U3, signal for remote wakeup and wait for the
>> @@ -2438,6 +2450,7 @@ static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int intf_id)
>>   		ret = __dwc3_gadget_wakeup(dwc, false);
>>   		if (ret) {
>>   			spin_unlock_irqrestore(&dwc->lock, flags);
>> +			pm_runtime_put_noidle(dwc->dev);
>>   			return -EINVAL;
>>   		}
>>   		dwc3_resume_gadget(dwc);
>> @@ -2452,6 +2465,7 @@ static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int intf_id)
>>   		dev_err(dwc->dev, "function remote wakeup failed, ret:%d\n", ret);
>>   
>>   	spin_unlock_irqrestore(&dwc->lock, flags);
>> +	pm_runtime_put_noidle(dwc->dev);
>>   
>>   	return ret;
>>   }
>> @@ -2732,21 +2746,23 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
>>   	/*
>>   	 * Avoid issuing a runtime resume if the device is already in the
>>   	 * suspended state during gadget disconnect.  DWC3 gadget was already
>> -	 * halted/stopped during runtime suspend.
>> +	 * halted/stopped during runtime suspend except for bus suspend case
>> +	 * where we would have skipped the controller halt.
>>   	 */
>>   	if (!is_on) {
>>   		pm_runtime_barrier(dwc->dev);
>> -		if (pm_runtime_suspended(dwc->dev))
>> +		if (pm_runtime_suspended(dwc->dev) && !dwc->connected)
> 
> dwc->connected will to to false on disconnect. I'm not sure if we can rely
> on this flag here.
> 
This is for handling gadget_pullup() when we are in bus suspend state 
(and thus runtime suspended) for cases where a usb composition switch is 
triggered. Like explained above we dont clear dwc->connected flag 
without resuming the driver first.

>>   			return 0;
>>   	}
>>   
>>   	/*
>>   	 * Check the return value for successful resume, or error.  For a
>>   	 * successful resume, the DWC3 runtime PM resume routine will handle
>> -	 * the run stop sequence, so avoid duplicate operations here.
>> +	 * the run stop sequence except for bus resume case, so avoid
>> +	 * duplicate operations here.
>>   	 */
>>   	ret = pm_runtime_get_sync(dwc->dev);
>> -	if (!ret || ret < 0) {
>> +	if ((!ret && !dwc->connected) || ret < 0) {
> 
> Why this check?
> 
In the resume operation above we would have skipped setting the run stop 
bit and other operations for bus suspend scenario. Hence we are not 
bailing out here if dwc->connected is true (which indicates that the 
resume operation was for cable connected case)

>>   		pm_runtime_put(dwc->dev);
>>   		if (ret < 0)
>>   			pm_runtime_set_suspended(dwc->dev);
>> @@ -4331,6 +4347,8 @@ static void dwc3_gadget_suspend_interrupt(struct dwc3 *dwc,
>>   	}
>>   
>>   	dwc->link_state = next;
>> +	pm_runtime_mark_last_busy(dwc->dev);
>> +	pm_request_autosuspend(dwc->dev);
> 
> Not here. You should do this in dwc3_suspend_gadget() after gadget driver
> has completed suspend.
> 

Since the goal was to trigger runtime suspend when bus suspend irq is 
received I have added it here. I see that dwc3_suspend_gadget is called 
in other instances as well (like U1,U2) and hence having it here 
specific to U3/L2 handler is better IMO.
>>   }
>>   
>>   static void dwc3_gadget_interrupt(struct dwc3 *dwc,
>> @@ -4718,7 +4736,15 @@ void dwc3_gadget_process_pending_events(struct dwc3 *dwc)
>>   {
>>   	if (dwc->pending_events) {
>>   		dwc3_interrupt(dwc->irq_gadget, dwc->ev_buf);
>> +		pm_runtime_put(dwc->dev);
> 
> Why the put here?
>

To balance the get() called when setting the pending_events flag in 
dwc3_check_event_buf()

	if (pm_runtime_suspended(dwc->dev)) {
		pm_runtime_get(dwc->dev);
		disable_irq_nosync(dwc->irq_gadget);
		dwc->pending_events = true;
		return IRQ_HANDLED;
	}

>>   		dwc->pending_events = false;
>>   		enable_irq(dwc->irq_gadget);
>> +		/*
>> +		 * We have only stored the pending events as part
>> +		 * of dwc3_interrupt() above, but those events are
>> +		 * not yet handled. So explicitly invoke the
>> +		 * interrupt handler for handling those events.
>> +		 */
>> +		dwc3_thread_interrupt(dwc->irq_gadget, dwc->ev_buf);
>>   	}
>>   }
> 

Re: [PATCH v3 3/3] usb: dwc3: Modify runtime pm ops to handle bus suspend

[Roger Quadros]

On 13/07/2023 01:57, Elson Serrao wrote:
> 
> 
> On 7/12/2023 6:46 AM, Roger Quadros wrote:
>>
>>
>> On 11/07/2023 20:43, Elson Roy Serrao wrote:
>>> The current implementation blocks the runtime pm operations when cable
>>> is connected. This would block platforms from entering system wide suspend
>>> during bus suspend scenario. Modify the runtime pm ops to handle bus
>>> suspend case for such platforms where the controller low power mode
>>> entry/exit is handled by the glue driver. This enablement is controlled
>>> through a dt property and platforms capable of detecting bus resume can
>>> benefit from this feature. Also modify the remote wakeup operations to
>>> trigger runtime resume before sending wakeup signal.
>>>
>>> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
>>> ---
>>>   drivers/usb/dwc3/core.c   | 26 ++++++++++++++++++++++---
>>>   drivers/usb/dwc3/core.h   |  3 +++
>>>   drivers/usb/dwc3/gadget.c | 40 ++++++++++++++++++++++++++++++++-------
>>>   3 files changed, 59 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>> index f6689b731718..898c0f68e190 100644
>>> --- a/drivers/usb/dwc3/core.c
>>> +++ b/drivers/usb/dwc3/core.c
>>> @@ -1534,6 +1534,9 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>>>       dwc->dis_split_quirk = device_property_read_bool(dev,
>>>                   "snps,dis-split-quirk");
>>>   +    dwc->allow_rtsusp_on_u3 = device_property_read_bool(dev,
>>> +                "snps,allow-rtsusp-on-u3");
>>> +
>>>       dwc->lpm_nyet_threshold = lpm_nyet_threshold;
>>>       dwc->tx_de_emphasis = tx_de_emphasis;
>>>   @@ -1984,11 +1987,21 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
>>>   {
>>>       unsigned long    flags;
>>>       u32 reg;
>>> +    int link_state;
>>>         switch (dwc->current_dr_role) {
>>>       case DWC3_GCTL_PRTCAP_DEVICE:
>>>           if (pm_runtime_suspended(dwc->dev))
>>>               break;
>>> +
>>> +        if (dwc->connected) {
>>> +            link_state = dwc3_gadget_get_link_state(dwc);
>>> +            /* bus suspend case */
>>> +            if (dwc->allow_rtsusp_on_u3 &&
>>> +                link_state == DWC3_LINK_STATE_U3> +                break;
>>
>> dwc3_suspend_common() is called both during runtime suspend and system sleep with
>> appropriate pm_msg_t argument.
>> Do you want the same behavior for both cases?
>>
> Thank you for your comments and feedback.
> The goal here is to trigger runtime suspend when bus suspend interrupt is received. If dwc3 runtime suspend is executed, then for system sleep dwc3_suspend_common() is a no-op as we have a pm_runtime_suspended(dwc->dev) check in place in the above code.
> The exception I can think of with these changes in place is below sequence
> USB bus suspended---->Runtime suspend fails OR is not invoked for some reason--->System sleep is triggered.
> 
> We can extend this change to above case as well. I believe that was the intention of your patch (i.e avoid gadget disconnect if usb bus is suspended for system sleep case)
> https://lore.kernel.org/linux-usb/20230320093447.32105-3-rogerq@kernel.org/
> 
> Please let me know your opinion on extending this series to system sleep also.

As dwc3_suspend_common()/dwc3_resume_common() is used for both runtime and
system suspend it would be nice if you test that system suspend is not broken.
Doesn't have to be enhanced.

> 
> 
>> You are not checking if the gadget driver was put into suspend state or not.
>> So, instead of just checking line state, should we also check that we processed
>> the suspend interrupt and gadget driver has been suspended as well?
>> i.e. add a SW flag dwc3->suspended and set it at end of dwc3_suspend_gadget()
>> and clear it at beginning of dwc3_resume_gadget().
>> Check for that flag in the above if condition.
>> i.e. if (dwc->connected && dwc->suspended)
>>
> 
> We already have flags for these purposes (dwc->link_state and dwc->suspended) that we set when suspend interrupt is received and clear during wakeup. We can use these flags here

OK but bear in mind that link_state might not be updated once runtime suspended.

> 
>>> +            return -EBUSY;
>>> +        }
>>>           dwc3_gadget_suspend(dwc);
>>>           synchronize_irq(dwc->irq_gadget);
>>>           dwc3_core_exit(dwc);
>>> @@ -2045,6 +2058,9 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
>>>         switch (dwc->current_dr_role) {
>>>       case DWC3_GCTL_PRTCAP_DEVICE:
>>> +        /* bus resume case */
>>> +        if (dwc->connected)
>>
>> dwc->connected might get cleard to false if device was disconnected. So this is not reliable.
>> You might want to set another SW flag to track this special case of suspend
>> with controller kept active.
>>
> 
> Are you referring to the DISCONNECT interrupt ? We dont handle gadget interrupts when in runtime suspended state and dwc3 driver has to be resumed first. The mechanism for this already exists in dwc3_check_event_buf() like below
> 
>     if (pm_runtime_suspended(dwc->dev)) {
>         pm_runtime_get(dwc->dev);
>         disable_irq_nosync(dwc->irq_gadget);
>         dwc->pending_events = true;
>         return IRQ_HANDLED;
>     }
> 
> I cant think of any scenario where dwc->connected is cleared before calling resume_common() for bus suspend case. Please let me know if I am missing any corner case.
> 
>>> +            break;
>>>           ret = dwc3_core_init_for_resume(dwc);
>>>           if (ret)
>>>               return ret;
>>> @@ -2123,9 +2139,6 @@ static int dwc3_runtime_suspend(struct device *dev)
>>>       struct dwc3     *dwc = dev_get_drvdata(dev);
>>>       int        ret;
>>>   -    if (dwc3_runtime_checks(dwc))
>>> -        return -EBUSY;
>>> -
>> Instead of removing this how about modifying dwc3_runtime_checks(). see below.
>>
> 
> Yes we can do that. But if we are planning to extend these changes to system sleep better to leave it here IMO.
> 
>>>       ret = dwc3_suspend_common(dwc, PMSG_AUTO_SUSPEND);
>>>       if (ret)
>>>           return ret;
>>> @@ -2160,9 +2173,15 @@ static int dwc3_runtime_resume(struct device *dev)
>>>   static int dwc3_runtime_idle(struct device *dev)
>>>   {
>>>       struct dwc3     *dwc = dev_get_drvdata(dev);
>>> +    int        link_state;
>>>         switch (dwc->current_dr_role) {
>>>       case DWC3_GCTL_PRTCAP_DEVICE:
>>> +        link_state = dwc3_gadget_get_link_state(dwc);
>>> +        /* for bus suspend case return success */
>>> +        if (dwc->allow_rtsusp_on_u3 && dwc->connected &&
>>> +            link_state == DWC3_LINK_STATE_U3)
>>> +            goto autosuspend;
>>
>> why not just break?
> Done
>>
>>>           if (dwc3_runtime_checks(dwc))
>>
>> what about this dwc3_runtime_checks()?
>> Looks like the if condition you added above can go in dwc3_runtime_checks() instead.
>>
>>>               return -EBUSY;
>>>           break;
>>> @@ -2172,6 +2191,7 @@ static int dwc3_runtime_idle(struct device *dev)
>>>           break;
>>>       }
>>>   +autosuspend:
>>>       pm_runtime_mark_last_busy(dev);
>>>       pm_runtime_autosuspend(dev);
>>>   diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>>> index 8b1295e4dcdd..33b2ccbbd963 100644
>>> --- a/drivers/usb/dwc3/core.h
>>> +++ b/drivers/usb/dwc3/core.h
>>> @@ -1127,6 +1127,8 @@ struct dwc3_scratchpad_array {
>>>    * @num_ep_resized: carries the current number endpoints which have had its tx
>>>    *            fifo resized.
>>>    * @debug_root: root debugfs directory for this device to put its files in.
>>> + * @allow_rtsusp_on_u3: true if dwc3 runtime suspend is allowed during bus
>>> + *            suspend scenario.
>>
>> what about system sleep?
>>
> Currently this series is tested for runtime suspend during bus suspend scenario. We can extend to system sleep like explained above.
> 
>>>    */
>>>   struct dwc3 {
>>>       struct work_struct    drd_work;
>>> @@ -1343,6 +1345,7 @@ struct dwc3 {
>>>       int            last_fifo_depth;
>>>       int            num_ep_resized;
>>>       struct dentry        *debug_root;
>>> +    bool            allow_rtsusp_on_u3;
>>>   };
>>>     #define INCRX_BURST_MODE 0
>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>> index 5fd067151fbf..0797cffa2d48 100644
>>> --- a/drivers/usb/dwc3/gadget.c
>>> +++ b/drivers/usb/dwc3/gadget.c
>>> @@ -2401,15 +2401,21 @@ static int dwc3_gadget_wakeup(struct usb_gadget *g)
>>>           return -EINVAL;
>>>       }
>>>   -    spin_lock_irqsave(&dwc->lock, flags);
>>>       if (!dwc->gadget->wakeup_armed) {
>>>           dev_err(dwc->dev, "not armed for remote wakeup\n");
>>> -        spin_unlock_irqrestore(&dwc->lock, flags);
>>>           return -EINVAL;
>>>       }
>>> -    ret = __dwc3_gadget_wakeup(dwc, true);
>>>   +    ret = pm_runtime_resume_and_get(dwc->dev);
>>> +    if (ret < 0) {
>>> +        pm_runtime_set_suspended(dwc->dev);
>>> +        return ret;
>>> +    }
>>> +
>>> +    spin_lock_irqsave(&dwc->lock, flags);
>>> +    ret = __dwc3_gadget_wakeup(dwc, true);
>>>       spin_unlock_irqrestore(&dwc->lock, flags);
>>> +    pm_runtime_put_noidle(dwc->dev);
>>>         return ret;
>>>   }
>>> @@ -2428,6 +2434,12 @@ static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int intf_id)
>>>           return -EINVAL;
>>>       }
>>>   +    ret = pm_runtime_resume_and_get(dwc->dev);
>>> +    if (ret < 0) {
>>> +        pm_runtime_set_suspended(dwc->dev);
>>> +        return ret;
>>> +    }
>>> +
>>>       spin_lock_irqsave(&dwc->lock, flags);
>>>       /*
>>>        * If the link is in U3, signal for remote wakeup and wait for the
>>> @@ -2438,6 +2450,7 @@ static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int intf_id)
>>>           ret = __dwc3_gadget_wakeup(dwc, false);
>>>           if (ret) {
>>>               spin_unlock_irqrestore(&dwc->lock, flags);
>>> +            pm_runtime_put_noidle(dwc->dev);
>>>               return -EINVAL;
>>>           }
>>>           dwc3_resume_gadget(dwc);
>>> @@ -2452,6 +2465,7 @@ static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int intf_id)
>>>           dev_err(dwc->dev, "function remote wakeup failed, ret:%d\n", ret);
>>>         spin_unlock_irqrestore(&dwc->lock, flags);
>>> +    pm_runtime_put_noidle(dwc->dev);
>>>         return ret;
>>>   }
>>> @@ -2732,21 +2746,23 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
>>>       /*
>>>        * Avoid issuing a runtime resume if the device is already in the
>>>        * suspended state during gadget disconnect.  DWC3 gadget was already
>>> -     * halted/stopped during runtime suspend.
>>> +     * halted/stopped during runtime suspend except for bus suspend case
>>> +     * where we would have skipped the controller halt.
>>>        */
>>>       if (!is_on) {
>>>           pm_runtime_barrier(dwc->dev);
>>> -        if (pm_runtime_suspended(dwc->dev))
>>> +        if (pm_runtime_suspended(dwc->dev) && !dwc->connected)
>>
>> dwc->connected will to to false on disconnect. I'm not sure if we can rely
>> on this flag here.
>>
> This is for handling gadget_pullup() when we are in bus suspend state (and thus runtime suspended) for cases where a usb composition switch is triggered. Like explained above we dont clear dwc->connected flag without resuming the driver first.
> 
>>>               return 0;
>>>       }
>>>         /*
>>>        * Check the return value for successful resume, or error.  For a
>>>        * successful resume, the DWC3 runtime PM resume routine will handle
>>> -     * the run stop sequence, so avoid duplicate operations here.
>>> +     * the run stop sequence except for bus resume case, so avoid
>>> +     * duplicate operations here.
>>>        */
>>>       ret = pm_runtime_get_sync(dwc->dev);
>>> -    if (!ret || ret < 0) {
>>> +    if ((!ret && !dwc->connected) || ret < 0) {
>>
>> Why this check?
>>
> In the resume operation above we would have skipped setting the run stop bit and other operations for bus suspend scenario. Hence we are not bailing out here if dwc->connected is true (which indicates that the resume operation was for cable connected case)

I think we are over-complicating the simple pullup() function.
How about not skipping setting the necessary bits in bus suspend case?

> 
>>>           pm_runtime_put(dwc->dev);
>>>           if (ret < 0)
>>>               pm_runtime_set_suspended(dwc->dev);
>>> @@ -4331,6 +4347,8 @@ static void dwc3_gadget_suspend_interrupt(struct dwc3 *dwc,
>>>       }
>>>         dwc->link_state = next;
>>> +    pm_runtime_mark_last_busy(dwc->dev);
>>> +    pm_request_autosuspend(dwc->dev);
>>
>> Not here. You should do this in dwc3_suspend_gadget() after gadget driver
>> has completed suspend.
>>
> 
> Since the goal was to trigger runtime suspend when bus suspend irq is received I have added it here. I see that dwc3_suspend_gadget is called in other instances as well (like U1,U2) and hence having it here specific to U3/L2 handler is better IMO.

OK.

>>>   }
>>>     static void dwc3_gadget_interrupt(struct dwc3 *dwc,
>>> @@ -4718,7 +4736,15 @@ void dwc3_gadget_process_pending_events(struct dwc3 *dwc)
>>>   {
>>>       if (dwc->pending_events) {
>>>           dwc3_interrupt(dwc->irq_gadget, dwc->ev_buf);
>>> +        pm_runtime_put(dwc->dev);
>>
>> Why the put here?
>>
> 
> To balance the get() called when setting the pending_events flag in dwc3_check_event_buf()
> 
>     if (pm_runtime_suspended(dwc->dev)) {
>         pm_runtime_get(dwc->dev);
>         disable_irq_nosync(dwc->irq_gadget);
>         dwc->pending_events = true;
>         return IRQ_HANDLED;
>     }
> 

No this wrong. We want the device to be active from now on.

runtime suspended->interrupt->pm_runtime_get->runtime_resume->process_pending_events->USB gadget resumed

Only on next USB suspend you want to do the pm_runtime_put like you are doing it
in dwc3_gadget_suspend_interrupt() by pm_request_autosuspend()

>>>           dwc->pending_events = false;
>>>           enable_irq(dwc->irq_gadget);
>>> +        /*
>>> +         * We have only stored the pending events as part
>>> +         * of dwc3_interrupt() above, but those events are
>>> +         * not yet handled. So explicitly invoke the
>>> +         * interrupt handler for handling those events.
>>> +         */
>>> +        dwc3_thread_interrupt(dwc->irq_gadget, dwc->ev_buf);
>>>       }
>>>   }
>>

-- 
cheers,
-roger

Re: [PATCH v3 3/3] usb: dwc3: Modify runtime pm ops to handle bus suspend

[Elson Serrao]

On 7/13/2023 5:56 AM, Roger Quadros wrote:
> 
> 
> On 13/07/2023 01:57, Elson Serrao wrote:
>>
>>
>> On 7/12/2023 6:46 AM, Roger Quadros wrote:
>>>
>>>
>>> On 11/07/2023 20:43, Elson Roy Serrao wrote:
>>>> The current implementation blocks the runtime pm operations when cable
>>>> is connected. This would block platforms from entering system wide suspend
>>>> during bus suspend scenario. Modify the runtime pm ops to handle bus
>>>> suspend case for such platforms where the controller low power mode
>>>> entry/exit is handled by the glue driver. This enablement is controlled
>>>> through a dt property and platforms capable of detecting bus resume can
>>>> benefit from this feature. Also modify the remote wakeup operations to
>>>> trigger runtime resume before sending wakeup signal.
>>>>
>>>> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
>>>> ---
>>>>    drivers/usb/dwc3/core.c   | 26 ++++++++++++++++++++++---
>>>>    drivers/usb/dwc3/core.h   |  3 +++
>>>>    drivers/usb/dwc3/gadget.c | 40 ++++++++++++++++++++++++++++++++-------
>>>>    3 files changed, 59 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>> index f6689b731718..898c0f68e190 100644
>>>> --- a/drivers/usb/dwc3/core.c
>>>> +++ b/drivers/usb/dwc3/core.c
>>>> @@ -1534,6 +1534,9 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>>>>        dwc->dis_split_quirk = device_property_read_bool(dev,
>>>>                    "snps,dis-split-quirk");
>>>>    +    dwc->allow_rtsusp_on_u3 = device_property_read_bool(dev,
>>>> +                "snps,allow-rtsusp-on-u3");
>>>> +
>>>>        dwc->lpm_nyet_threshold = lpm_nyet_threshold;
>>>>        dwc->tx_de_emphasis = tx_de_emphasis;
>>>>    @@ -1984,11 +1987,21 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
>>>>    {
>>>>        unsigned long    flags;
>>>>        u32 reg;
>>>> +    int link_state;
>>>>          switch (dwc->current_dr_role) {
>>>>        case DWC3_GCTL_PRTCAP_DEVICE:
>>>>            if (pm_runtime_suspended(dwc->dev))
>>>>                break;
>>>> +
>>>> +        if (dwc->connected) {
>>>> +            link_state = dwc3_gadget_get_link_state(dwc);
>>>> +            /* bus suspend case */
>>>> +            if (dwc->allow_rtsusp_on_u3 &&
>>>> +                link_state == DWC3_LINK_STATE_U3> +                break;
>>>
>>> dwc3_suspend_common() is called both during runtime suspend and system sleep with
>>> appropriate pm_msg_t argument.
>>> Do you want the same behavior for both cases?
>>>
>> Thank you for your comments and feedback.
>> The goal here is to trigger runtime suspend when bus suspend interrupt is received. If dwc3 runtime suspend is executed, then for system sleep dwc3_suspend_common() is a no-op as we have a pm_runtime_suspended(dwc->dev) check in place in the above code.
>> The exception I can think of with these changes in place is below sequence
>> USB bus suspended---->Runtime suspend fails OR is not invoked for some reason--->System sleep is triggered.
>>
>> We can extend this change to above case as well. I believe that was the intention of your patch (i.e avoid gadget disconnect if usb bus is suspended for system sleep case)
>> https://lore.kernel.org/linux-usb/20230320093447.32105-3-rogerq@kernel.org/
>>
>> Please let me know your opinion on extending this series to system sleep also.
> 
> As dwc3_suspend_common()/dwc3_resume_common() is used for both runtime and
> system suspend it would be nice if you test that system suspend is not broken.
> Doesn't have to be enhanced.
> 
Sure. Will update the details accordingly in v4.
>>
>>
>>> You are not checking if the gadget driver was put into suspend state or not.
>>> So, instead of just checking line state, should we also check that we processed
>>> the suspend interrupt and gadget driver has been suspended as well?
>>> i.e. add a SW flag dwc3->suspended and set it at end of dwc3_suspend_gadget()
>>> and clear it at beginning of dwc3_resume_gadget().
>>> Check for that flag in the above if condition.
>>> i.e. if (dwc->connected && dwc->suspended)
>>>
>>
>> We already have flags for these purposes (dwc->link_state and dwc->suspended) that we set when suspend interrupt is received and clear during wakeup. We can use these flags here
> 
> OK but bear in mind that link_state might not be updated once runtime suspended.
> 
>>
>>>> +            return -EBUSY;
>>>> +        }
>>>>            dwc3_gadget_suspend(dwc);
>>>>            synchronize_irq(dwc->irq_gadget);
>>>>            dwc3_core_exit(dwc);
>>>> @@ -2045,6 +2058,9 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
>>>>          switch (dwc->current_dr_role) {
>>>>        case DWC3_GCTL_PRTCAP_DEVICE:
>>>> +        /* bus resume case */
>>>> +        if (dwc->connected)
>>>
>>> dwc->connected might get cleard to false if device was disconnected. So this is not reliable.
>>> You might want to set another SW flag to track this special case of suspend
>>> with controller kept active.
>>>
>>
>> Are you referring to the DISCONNECT interrupt ? We dont handle gadget interrupts when in runtime suspended state and dwc3 driver has to be resumed first. The mechanism for this already exists in dwc3_check_event_buf() like below
>>
>>      if (pm_runtime_suspended(dwc->dev)) {
>>          pm_runtime_get(dwc->dev);
>>          disable_irq_nosync(dwc->irq_gadget);
>>          dwc->pending_events = true;
>>          return IRQ_HANDLED;
>>      }
>>
>> I cant think of any scenario where dwc->connected is cleared before calling resume_common() for bus suspend case. Please let me know if I am missing any corner case.
>>
>>>> +            break;
>>>>            ret = dwc3_core_init_for_resume(dwc);
>>>>            if (ret)
>>>>                return ret;
>>>> @@ -2123,9 +2139,6 @@ static int dwc3_runtime_suspend(struct device *dev)
>>>>        struct dwc3     *dwc = dev_get_drvdata(dev);
>>>>        int        ret;
>>>>    -    if (dwc3_runtime_checks(dwc))
>>>> -        return -EBUSY;
>>>> -
>>> Instead of removing this how about modifying dwc3_runtime_checks(). see below.
>>>
>>
>> Yes we can do that. But if we are planning to extend these changes to system sleep better to leave it here IMO.
>>
>>>>        ret = dwc3_suspend_common(dwc, PMSG_AUTO_SUSPEND);
>>>>        if (ret)
>>>>            return ret;
>>>> @@ -2160,9 +2173,15 @@ static int dwc3_runtime_resume(struct device *dev)
>>>>    static int dwc3_runtime_idle(struct device *dev)
>>>>    {
>>>>        struct dwc3     *dwc = dev_get_drvdata(dev);
>>>> +    int        link_state;
>>>>          switch (dwc->current_dr_role) {
>>>>        case DWC3_GCTL_PRTCAP_DEVICE:
>>>> +        link_state = dwc3_gadget_get_link_state(dwc);
>>>> +        /* for bus suspend case return success */
>>>> +        if (dwc->allow_rtsusp_on_u3 && dwc->connected &&
>>>> +            link_state == DWC3_LINK_STATE_U3)
>>>> +            goto autosuspend;
>>>
>>> why not just break?
>> Done
>>>
>>>>            if (dwc3_runtime_checks(dwc))
>>>
>>> what about this dwc3_runtime_checks()?
>>> Looks like the if condition you added above can go in dwc3_runtime_checks() instead.
>>>
>>>>                return -EBUSY;
>>>>            break;
>>>> @@ -2172,6 +2191,7 @@ static int dwc3_runtime_idle(struct device *dev)
>>>>            break;
>>>>        }
>>>>    +autosuspend:
>>>>        pm_runtime_mark_last_busy(dev);
>>>>        pm_runtime_autosuspend(dev);
>>>>    diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>>>> index 8b1295e4dcdd..33b2ccbbd963 100644
>>>> --- a/drivers/usb/dwc3/core.h
>>>> +++ b/drivers/usb/dwc3/core.h
>>>> @@ -1127,6 +1127,8 @@ struct dwc3_scratchpad_array {
>>>>     * @num_ep_resized: carries the current number endpoints which have had its tx
>>>>     *            fifo resized.
>>>>     * @debug_root: root debugfs directory for this device to put its files in.
>>>> + * @allow_rtsusp_on_u3: true if dwc3 runtime suspend is allowed during bus
>>>> + *            suspend scenario.
>>>
>>> what about system sleep?
>>>
>> Currently this series is tested for runtime suspend during bus suspend scenario. We can extend to system sleep like explained above.
>>
>>>>     */
>>>>    struct dwc3 {
>>>>        struct work_struct    drd_work;
>>>> @@ -1343,6 +1345,7 @@ struct dwc3 {
>>>>        int            last_fifo_depth;
>>>>        int            num_ep_resized;
>>>>        struct dentry        *debug_root;
>>>> +    bool            allow_rtsusp_on_u3;
>>>>    };
>>>>      #define INCRX_BURST_MODE 0
>>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>>> index 5fd067151fbf..0797cffa2d48 100644
>>>> --- a/drivers/usb/dwc3/gadget.c
>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>> @@ -2401,15 +2401,21 @@ static int dwc3_gadget_wakeup(struct usb_gadget *g)
>>>>            return -EINVAL;
>>>>        }
>>>>    -    spin_lock_irqsave(&dwc->lock, flags);
>>>>        if (!dwc->gadget->wakeup_armed) {
>>>>            dev_err(dwc->dev, "not armed for remote wakeup\n");
>>>> -        spin_unlock_irqrestore(&dwc->lock, flags);
>>>>            return -EINVAL;
>>>>        }
>>>> -    ret = __dwc3_gadget_wakeup(dwc, true);
>>>>    +    ret = pm_runtime_resume_and_get(dwc->dev);
>>>> +    if (ret < 0) {
>>>> +        pm_runtime_set_suspended(dwc->dev);
>>>> +        return ret;
>>>> +    }
>>>> +
>>>> +    spin_lock_irqsave(&dwc->lock, flags);
>>>> +    ret = __dwc3_gadget_wakeup(dwc, true);
>>>>        spin_unlock_irqrestore(&dwc->lock, flags);
>>>> +    pm_runtime_put_noidle(dwc->dev);
>>>>          return ret;
>>>>    }
>>>> @@ -2428,6 +2434,12 @@ static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int intf_id)
>>>>            return -EINVAL;
>>>>        }
>>>>    +    ret = pm_runtime_resume_and_get(dwc->dev);
>>>> +    if (ret < 0) {
>>>> +        pm_runtime_set_suspended(dwc->dev);
>>>> +        return ret;
>>>> +    }
>>>> +
>>>>        spin_lock_irqsave(&dwc->lock, flags);
>>>>        /*
>>>>         * If the link is in U3, signal for remote wakeup and wait for the
>>>> @@ -2438,6 +2450,7 @@ static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int intf_id)
>>>>            ret = __dwc3_gadget_wakeup(dwc, false);
>>>>            if (ret) {
>>>>                spin_unlock_irqrestore(&dwc->lock, flags);
>>>> +            pm_runtime_put_noidle(dwc->dev);
>>>>                return -EINVAL;
>>>>            }
>>>>            dwc3_resume_gadget(dwc);
>>>> @@ -2452,6 +2465,7 @@ static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int intf_id)
>>>>            dev_err(dwc->dev, "function remote wakeup failed, ret:%d\n", ret);
>>>>          spin_unlock_irqrestore(&dwc->lock, flags);
>>>> +    pm_runtime_put_noidle(dwc->dev);
>>>>          return ret;
>>>>    }
>>>> @@ -2732,21 +2746,23 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
>>>>        /*
>>>>         * Avoid issuing a runtime resume if the device is already in the
>>>>         * suspended state during gadget disconnect.  DWC3 gadget was already
>>>> -     * halted/stopped during runtime suspend.
>>>> +     * halted/stopped during runtime suspend except for bus suspend case
>>>> +     * where we would have skipped the controller halt.
>>>>         */
>>>>        if (!is_on) {
>>>>            pm_runtime_barrier(dwc->dev);
>>>> -        if (pm_runtime_suspended(dwc->dev))
>>>> +        if (pm_runtime_suspended(dwc->dev) && !dwc->connected)
>>>
>>> dwc->connected will to to false on disconnect. I'm not sure if we can rely
>>> on this flag here.
>>>
>> This is for handling gadget_pullup() when we are in bus suspend state (and thus runtime suspended) for cases where a usb composition switch is triggered. Like explained above we dont clear dwc->connected flag without resuming the driver first.
>>
>>>>                return 0;
>>>>        }
>>>>          /*
>>>>         * Check the return value for successful resume, or error.  For a
>>>>         * successful resume, the DWC3 runtime PM resume routine will handle
>>>> -     * the run stop sequence, so avoid duplicate operations here.
>>>> +     * the run stop sequence except for bus resume case, so avoid
>>>> +     * duplicate operations here.
>>>>         */
>>>>        ret = pm_runtime_get_sync(dwc->dev);
>>>> -    if (!ret || ret < 0) {
>>>> +    if ((!ret && !dwc->connected) || ret < 0) {
>>>
>>> Why this check?
>>>
>> In the resume operation above we would have skipped setting the run stop bit and other operations for bus suspend scenario. Hence we are not bailing out here if dwc->connected is true (which indicates that the resume operation was for cable connected case)
> 
> I think we are over-complicating the simple pullup() function.
> How about not skipping setting the necessary bits in bus suspend case?

We cannot clear the RUN_STOP bit during bus suspend as that would stop 
the controller and disconnect from the bus. (I believe the exception 
would be if HIBERNATION feature is enabled on dwc3 IP).

I am failing to see the complication here.
The current implementation is to skip the run stop sequence in pullup() 
function if resume is triggered. The addition here is, not to skip the 
run stop sequence in pullup() if resume is triggered in bus suspend context.

>>
>>>>            pm_runtime_put(dwc->dev);
>>>>            if (ret < 0)
>>>>                pm_runtime_set_suspended(dwc->dev);
>>>> @@ -4331,6 +4347,8 @@ static void dwc3_gadget_suspend_interrupt(struct dwc3 *dwc,
>>>>        }
>>>>          dwc->link_state = next;
>>>> +    pm_runtime_mark_last_busy(dwc->dev);
>>>> +    pm_request_autosuspend(dwc->dev);
>>>
>>> Not here. You should do this in dwc3_suspend_gadget() after gadget driver
>>> has completed suspend.
>>>
>>
>> Since the goal was to trigger runtime suspend when bus suspend irq is received I have added it here. I see that dwc3_suspend_gadget is called in other instances as well (like U1,U2) and hence having it here specific to U3/L2 handler is better IMO.
> 
> OK.
> 
>>>>    }
>>>>      static void dwc3_gadget_interrupt(struct dwc3 *dwc,
>>>> @@ -4718,7 +4736,15 @@ void dwc3_gadget_process_pending_events(struct dwc3 *dwc)
>>>>    {
>>>>        if (dwc->pending_events) {
>>>>            dwc3_interrupt(dwc->irq_gadget, dwc->ev_buf);
>>>> +        pm_runtime_put(dwc->dev);
>>>
>>> Why the put here?
>>>
>>
>> To balance the get() called when setting the pending_events flag in dwc3_check_event_buf()
>>
>>      if (pm_runtime_suspended(dwc->dev)) {
>>          pm_runtime_get(dwc->dev);
>>          disable_irq_nosync(dwc->irq_gadget);
>>          dwc->pending_events = true;
>>          return IRQ_HANDLED;
>>      }
>>
> 
> No this wrong. We want the device to be active from now on.
> 
> runtime suspended->interrupt->pm_runtime_get->runtime_resume->process_pending_events->USB gadget resumed
> 
> Only on next USB suspend you want to do the pm_runtime_put like you are doing it
> in dwc3_gadget_suspend_interrupt() by pm_request_autosuspend()
> 

That would break/block dwc3 runtime suspend during DISCONNECT case in 
below scenario

runtime suspended->interrupt->pm_runtime_get (runtime usage count is 
1)->runtime_resume->process_pending_events->USB gadget resumed -> USB 
disconnect (autosuspend blocked due to runtime usage count being 1 due 
to unbalanced get() ).

The idea here is to balance the get() that was requested for processing 
the pending events, after processing those events. (like how we balance 
get() of ep_queue through put() in ep_dequeue)

Also pm_request_autosuspend() doesnt decrement the usage count, it only 
requests for autosuspend.

But I think better approach in terms of ordering is below

@@ -4718,7 +4736,15 @@ void dwc3_gadget_process_pending_events(struct 
dwc3 *dwc)
  {
  	if (dwc->pending_events) {
  		dwc3_interrupt(dwc->irq_gadget, dwc->ev_buf);
+		/*
+		 * We have only stored the pending events as part
+		 * of dwc3_interrupt() above, but those events are
+		 * not yet handled. So explicitly invoke the
+		 * interrupt handler for handling those events.
+		 */
+		dwc3_thread_interrupt(dwc->irq_gadget, dwc->ev_buf);
  		dwc->pending_events = false;
  		enable_irq(dwc->irq_gadget);
+		pm_runtime_put(dwc->dev);

  	}
  }

Regards
Elson

Re: [PATCH v3 3/3] usb: dwc3: Modify runtime pm ops to handle bus suspend

[Roger Quadros]

On 14/07/2023 01:31, Elson Serrao wrote:
> 
> 
> On 7/13/2023 5:56 AM, Roger Quadros wrote:
>>
>>
>> On 13/07/2023 01:57, Elson Serrao wrote:
>>>
>>>
>>> On 7/12/2023 6:46 AM, Roger Quadros wrote:
>>>>
>>>>
>>>> On 11/07/2023 20:43, Elson Roy Serrao wrote:
>>>>> The current implementation blocks the runtime pm operations when cable
>>>>> is connected. This would block platforms from entering system wide suspend
>>>>> during bus suspend scenario. Modify the runtime pm ops to handle bus
>>>>> suspend case for such platforms where the controller low power mode
>>>>> entry/exit is handled by the glue driver. This enablement is controlled
>>>>> through a dt property and platforms capable of detecting bus resume can
>>>>> benefit from this feature. Also modify the remote wakeup operations to
>>>>> trigger runtime resume before sending wakeup signal.
>>>>>
>>>>> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
>>>>> ---
>>>>>    drivers/usb/dwc3/core.c   | 26 ++++++++++++++++++++++---
>>>>>    drivers/usb/dwc3/core.h   |  3 +++
>>>>>    drivers/usb/dwc3/gadget.c | 40 ++++++++++++++++++++++++++++++++-------
>>>>>    3 files changed, 59 insertions(+), 10 deletions(-)
>>>>>
>>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>>> index f6689b731718..898c0f68e190 100644
>>>>> --- a/drivers/usb/dwc3/core.c
>>>>> +++ b/drivers/usb/dwc3/core.c
>>>>> @@ -1534,6 +1534,9 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>>>>>        dwc->dis_split_quirk = device_property_read_bool(dev,
>>>>>                    "snps,dis-split-quirk");
>>>>>    +    dwc->allow_rtsusp_on_u3 = device_property_read_bool(dev,
>>>>> +                "snps,allow-rtsusp-on-u3");
>>>>> +
>>>>>        dwc->lpm_nyet_threshold = lpm_nyet_threshold;
>>>>>        dwc->tx_de_emphasis = tx_de_emphasis;
>>>>>    @@ -1984,11 +1987,21 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
>>>>>    {
>>>>>        unsigned long    flags;
>>>>>        u32 reg;
>>>>> +    int link_state;
>>>>>          switch (dwc->current_dr_role) {
>>>>>        case DWC3_GCTL_PRTCAP_DEVICE:
>>>>>            if (pm_runtime_suspended(dwc->dev))
>>>>>                break;
>>>>> +
>>>>> +        if (dwc->connected) {
>>>>> +            link_state = dwc3_gadget_get_link_state(dwc);
>>>>> +            /* bus suspend case */
>>>>> +            if (dwc->allow_rtsusp_on_u3 &&
>>>>> +                link_state == DWC3_LINK_STATE_U3> +                break;
>>>>
>>>> dwc3_suspend_common() is called both during runtime suspend and system sleep with
>>>> appropriate pm_msg_t argument.
>>>> Do you want the same behavior for both cases?
>>>>
>>> Thank you for your comments and feedback.
>>> The goal here is to trigger runtime suspend when bus suspend interrupt is received. If dwc3 runtime suspend is executed, then for system sleep dwc3_suspend_common() is a no-op as we have a pm_runtime_suspended(dwc->dev) check in place in the above code.
>>> The exception I can think of with these changes in place is below sequence
>>> USB bus suspended---->Runtime suspend fails OR is not invoked for some reason--->System sleep is triggered.
>>>
>>> We can extend this change to above case as well. I believe that was the intention of your patch (i.e avoid gadget disconnect if usb bus is suspended for system sleep case)
>>> https://lore.kernel.org/linux-usb/20230320093447.32105-3-rogerq@kernel.org/
>>>
>>> Please let me know your opinion on extending this series to system sleep also.
>>
>> As dwc3_suspend_common()/dwc3_resume_common() is used for both runtime and
>> system suspend it would be nice if you test that system suspend is not broken.
>> Doesn't have to be enhanced.
>>
> Sure. Will update the details accordingly in v4.
>>>
>>>
>>>> You are not checking if the gadget driver was put into suspend state or not.
>>>> So, instead of just checking line state, should we also check that we processed
>>>> the suspend interrupt and gadget driver has been suspended as well?
>>>> i.e. add a SW flag dwc3->suspended and set it at end of dwc3_suspend_gadget()
>>>> and clear it at beginning of dwc3_resume_gadget().
>>>> Check for that flag in the above if condition.
>>>> i.e. if (dwc->connected && dwc->suspended)
>>>>
>>>
>>> We already have flags for these purposes (dwc->link_state and dwc->suspended) that we set when suspend interrupt is received and clear during wakeup. We can use these flags here
>>
>> OK but bear in mind that link_state might not be updated once runtime suspended.
>>
>>>
>>>>> +            return -EBUSY;
>>>>> +        }
>>>>>            dwc3_gadget_suspend(dwc);
>>>>>            synchronize_irq(dwc->irq_gadget);
>>>>>            dwc3_core_exit(dwc);
>>>>> @@ -2045,6 +2058,9 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
>>>>>          switch (dwc->current_dr_role) {
>>>>>        case DWC3_GCTL_PRTCAP_DEVICE:
>>>>> +        /* bus resume case */
>>>>> +        if (dwc->connected)
>>>>
>>>> dwc->connected might get cleard to false if device was disconnected. So this is not reliable.
>>>> You might want to set another SW flag to track this special case of suspend
>>>> with controller kept active.
>>>>
>>>
>>> Are you referring to the DISCONNECT interrupt ? We dont handle gadget interrupts when in runtime suspended state and dwc3 driver has to be resumed first. The mechanism for this already exists in dwc3_check_event_buf() like below
>>>
>>>      if (pm_runtime_suspended(dwc->dev)) {
>>>          pm_runtime_get(dwc->dev);
>>>          disable_irq_nosync(dwc->irq_gadget);
>>>          dwc->pending_events = true;
>>>          return IRQ_HANDLED;
>>>      }
>>>
>>> I cant think of any scenario where dwc->connected is cleared before calling resume_common() for bus suspend case. Please let me know if I am missing any corner case.
>>>
>>>>> +            break;
>>>>>            ret = dwc3_core_init_for_resume(dwc);
>>>>>            if (ret)
>>>>>                return ret;
>>>>> @@ -2123,9 +2139,6 @@ static int dwc3_runtime_suspend(struct device *dev)
>>>>>        struct dwc3     *dwc = dev_get_drvdata(dev);
>>>>>        int        ret;
>>>>>    -    if (dwc3_runtime_checks(dwc))
>>>>> -        return -EBUSY;
>>>>> -
>>>> Instead of removing this how about modifying dwc3_runtime_checks(). see below.
>>>>
>>>
>>> Yes we can do that. But if we are planning to extend these changes to system sleep better to leave it here IMO.
>>>
>>>>>        ret = dwc3_suspend_common(dwc, PMSG_AUTO_SUSPEND);
>>>>>        if (ret)
>>>>>            return ret;
>>>>> @@ -2160,9 +2173,15 @@ static int dwc3_runtime_resume(struct device *dev)
>>>>>    static int dwc3_runtime_idle(struct device *dev)
>>>>>    {
>>>>>        struct dwc3     *dwc = dev_get_drvdata(dev);
>>>>> +    int        link_state;
>>>>>          switch (dwc->current_dr_role) {
>>>>>        case DWC3_GCTL_PRTCAP_DEVICE:
>>>>> +        link_state = dwc3_gadget_get_link_state(dwc);
>>>>> +        /* for bus suspend case return success */
>>>>> +        if (dwc->allow_rtsusp_on_u3 && dwc->connected &&
>>>>> +            link_state == DWC3_LINK_STATE_U3)
>>>>> +            goto autosuspend;
>>>>
>>>> why not just break?
>>> Done
>>>>
>>>>>            if (dwc3_runtime_checks(dwc))
>>>>
>>>> what about this dwc3_runtime_checks()?
>>>> Looks like the if condition you added above can go in dwc3_runtime_checks() instead.
>>>>
>>>>>                return -EBUSY;
>>>>>            break;
>>>>> @@ -2172,6 +2191,7 @@ static int dwc3_runtime_idle(struct device *dev)
>>>>>            break;
>>>>>        }
>>>>>    +autosuspend:
>>>>>        pm_runtime_mark_last_busy(dev);
>>>>>        pm_runtime_autosuspend(dev);
>>>>>    diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>>>>> index 8b1295e4dcdd..33b2ccbbd963 100644
>>>>> --- a/drivers/usb/dwc3/core.h
>>>>> +++ b/drivers/usb/dwc3/core.h
>>>>> @@ -1127,6 +1127,8 @@ struct dwc3_scratchpad_array {
>>>>>     * @num_ep_resized: carries the current number endpoints which have had its tx
>>>>>     *            fifo resized.
>>>>>     * @debug_root: root debugfs directory for this device to put its files in.
>>>>> + * @allow_rtsusp_on_u3: true if dwc3 runtime suspend is allowed during bus
>>>>> + *            suspend scenario.
>>>>
>>>> what about system sleep?
>>>>
>>> Currently this series is tested for runtime suspend during bus suspend scenario. We can extend to system sleep like explained above.
>>>
>>>>>     */
>>>>>    struct dwc3 {
>>>>>        struct work_struct    drd_work;
>>>>> @@ -1343,6 +1345,7 @@ struct dwc3 {
>>>>>        int            last_fifo_depth;
>>>>>        int            num_ep_resized;
>>>>>        struct dentry        *debug_root;
>>>>> +    bool            allow_rtsusp_on_u3;
>>>>>    };
>>>>>      #define INCRX_BURST_MODE 0
>>>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>>>> index 5fd067151fbf..0797cffa2d48 100644
>>>>> --- a/drivers/usb/dwc3/gadget.c
>>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>>> @@ -2401,15 +2401,21 @@ static int dwc3_gadget_wakeup(struct usb_gadget *g)
>>>>>            return -EINVAL;
>>>>>        }
>>>>>    -    spin_lock_irqsave(&dwc->lock, flags);
>>>>>        if (!dwc->gadget->wakeup_armed) {
>>>>>            dev_err(dwc->dev, "not armed for remote wakeup\n");
>>>>> -        spin_unlock_irqrestore(&dwc->lock, flags);
>>>>>            return -EINVAL;
>>>>>        }
>>>>> -    ret = __dwc3_gadget_wakeup(dwc, true);
>>>>>    +    ret = pm_runtime_resume_and_get(dwc->dev);
>>>>> +    if (ret < 0) {
>>>>> +        pm_runtime_set_suspended(dwc->dev);
>>>>> +        return ret;
>>>>> +    }
>>>>> +
>>>>> +    spin_lock_irqsave(&dwc->lock, flags);
>>>>> +    ret = __dwc3_gadget_wakeup(dwc, true);
>>>>>        spin_unlock_irqrestore(&dwc->lock, flags);
>>>>> +    pm_runtime_put_noidle(dwc->dev);
>>>>>          return ret;
>>>>>    }
>>>>> @@ -2428,6 +2434,12 @@ static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int intf_id)
>>>>>            return -EINVAL;
>>>>>        }
>>>>>    +    ret = pm_runtime_resume_and_get(dwc->dev);
>>>>> +    if (ret < 0) {
>>>>> +        pm_runtime_set_suspended(dwc->dev);
>>>>> +        return ret;
>>>>> +    }
>>>>> +
>>>>>        spin_lock_irqsave(&dwc->lock, flags);
>>>>>        /*
>>>>>         * If the link is in U3, signal for remote wakeup and wait for the
>>>>> @@ -2438,6 +2450,7 @@ static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int intf_id)
>>>>>            ret = __dwc3_gadget_wakeup(dwc, false);
>>>>>            if (ret) {
>>>>>                spin_unlock_irqrestore(&dwc->lock, flags);
>>>>> +            pm_runtime_put_noidle(dwc->dev);
>>>>>                return -EINVAL;
>>>>>            }
>>>>>            dwc3_resume_gadget(dwc);
>>>>> @@ -2452,6 +2465,7 @@ static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int intf_id)
>>>>>            dev_err(dwc->dev, "function remote wakeup failed, ret:%d\n", ret);
>>>>>          spin_unlock_irqrestore(&dwc->lock, flags);
>>>>> +    pm_runtime_put_noidle(dwc->dev);
>>>>>          return ret;
>>>>>    }
>>>>> @@ -2732,21 +2746,23 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
>>>>>        /*
>>>>>         * Avoid issuing a runtime resume if the device is already in the
>>>>>         * suspended state during gadget disconnect.  DWC3 gadget was already
>>>>> -     * halted/stopped during runtime suspend.
>>>>> +     * halted/stopped during runtime suspend except for bus suspend case
>>>>> +     * where we would have skipped the controller halt.
>>>>>         */
>>>>>        if (!is_on) {
>>>>>            pm_runtime_barrier(dwc->dev);
>>>>> -        if (pm_runtime_suspended(dwc->dev))
>>>>> +        if (pm_runtime_suspended(dwc->dev) && !dwc->connected)
>>>>
>>>> dwc->connected will to to false on disconnect. I'm not sure if we can rely
>>>> on this flag here.
>>>>
>>> This is for handling gadget_pullup() when we are in bus suspend state (and thus runtime suspended) for cases where a usb composition switch is triggered. Like explained above we dont clear dwc->connected flag without resuming the driver first.
>>>
>>>>>                return 0;
>>>>>        }
>>>>>          /*
>>>>>         * Check the return value for successful resume, or error.  For a
>>>>>         * successful resume, the DWC3 runtime PM resume routine will handle
>>>>> -     * the run stop sequence, so avoid duplicate operations here.
>>>>> +     * the run stop sequence except for bus resume case, so avoid
>>>>> +     * duplicate operations here.
>>>>>         */
>>>>>        ret = pm_runtime_get_sync(dwc->dev);
>>>>> -    if (!ret || ret < 0) {
>>>>> +    if ((!ret && !dwc->connected) || ret < 0) {
>>>>
>>>> Why this check?
>>>>
>>> In the resume operation above we would have skipped setting the run stop bit and other operations for bus suspend scenario. Hence we are not bailing out here if dwc->connected is true (which indicates that the resume operation was for cable connected case)
>>
>> I think we are over-complicating the simple pullup() function.
>> How about not skipping setting the necessary bits in bus suspend case?
> 
> We cannot clear the RUN_STOP bit during bus suspend as that would stop the controller and disconnect from the bus. (I believe the exception would be if HIBERNATION feature is enabled on dwc3 IP).
> 
> I am failing to see the complication here.
> The current implementation is to skip the run stop sequence in pullup() function if resume is triggered. The addition here is, not to skip the run stop sequence in pullup() if resume is triggered in bus suspend context.
> 
>>>
>>>>>            pm_runtime_put(dwc->dev);
>>>>>            if (ret < 0)
>>>>>                pm_runtime_set_suspended(dwc->dev);
>>>>> @@ -4331,6 +4347,8 @@ static void dwc3_gadget_suspend_interrupt(struct dwc3 *dwc,
>>>>>        }
>>>>>          dwc->link_state = next;
>>>>> +    pm_runtime_mark_last_busy(dwc->dev);
>>>>> +    pm_request_autosuspend(dwc->dev);
>>>>
>>>> Not here. You should do this in dwc3_suspend_gadget() after gadget driver
>>>> has completed suspend.
>>>>
>>>
>>> Since the goal was to trigger runtime suspend when bus suspend irq is received I have added it here. I see that dwc3_suspend_gadget is called in other instances as well (like U1,U2) and hence having it here specific to U3/L2 handler is better IMO.
>>
>> OK.
>>
>>>>>    }
>>>>>      static void dwc3_gadget_interrupt(struct dwc3 *dwc,
>>>>> @@ -4718,7 +4736,15 @@ void dwc3_gadget_process_pending_events(struct dwc3 *dwc)
>>>>>    {
>>>>>        if (dwc->pending_events) {
>>>>>            dwc3_interrupt(dwc->irq_gadget, dwc->ev_buf);
>>>>> +        pm_runtime_put(dwc->dev);
>>>>
>>>> Why the put here?
>>>>
>>>
>>> To balance the get() called when setting the pending_events flag in dwc3_check_event_buf()
>>>
>>>      if (pm_runtime_suspended(dwc->dev)) {
>>>          pm_runtime_get(dwc->dev);
>>>          disable_irq_nosync(dwc->irq_gadget);
>>>          dwc->pending_events = true;
>>>          return IRQ_HANDLED;
>>>      }
>>>
>>
>> No this wrong. We want the device to be active from now on.
>>
>> runtime suspended->interrupt->pm_runtime_get->runtime_resume->process_pending_events->USB gadget resumed
>>
>> Only on next USB suspend you want to do the pm_runtime_put like you are doing it
>> in dwc3_gadget_suspend_interrupt() by pm_request_autosuspend()
>>
> 
> That would break/block dwc3 runtime suspend during DISCONNECT case in below scenario
> 
> runtime suspended->interrupt->pm_runtime_get (runtime usage count is 1)->runtime_resume->process_pending_events->USB gadget resumed -> USB disconnect (autosuspend blocked due to runtime usage count being 1 due to unbalanced get() ).
> 
> The idea here is to balance the get() that was requested for processing the pending events, after processing those events. (like how we balance get() of ep_queue through put() in ep_dequeue)
> 
> Also pm_request_autosuspend() doesnt decrement the usage count, it only requests for autosuspend.

Ah, indeed.

> 
> But I think better approach in terms of ordering is below

ok, but should dwc->pending_events be set before calling pm_runtime_get() in dwc3_check_event_buf()?
Can we add a comment there that the get will be balanced out in dwc3_gadget_process_pending_events()?

> 
> @@ -4718,7 +4736,15 @@ void dwc3_gadget_process_pending_events(struct dwc3 *dwc)
>  {
>      if (dwc->pending_events) {
>          dwc3_interrupt(dwc->irq_gadget, dwc->ev_buf);
> +        /*
> +         * We have only stored the pending events as part
> +         * of dwc3_interrupt() above, but those events are
> +         * not yet handled. So explicitly invoke the
> +         * interrupt handler for handling those events.
> +         */
> +        dwc3_thread_interrupt(dwc->irq_gadget, dwc->ev_buf);
>          dwc->pending_events = false;
>          enable_irq(dwc->irq_gadget);
> +        pm_runtime_put(dwc->dev);

We could do the put right after dwc3_thread_interrupt().

> 
>      }
>  }

I think this fix should be an independent patch
as this fixes an issue that existed prior to this series?
also need to -cc stable?

-- 
cheers,
-roger

Re: [PATCH v3 3/3] usb: dwc3: Modify runtime pm ops to handle bus suspend

[Elson Serrao]

On 7/14/2023 5:23 AM, Roger Quadros wrote:
> 
> 

>>>>>>     }
>>>>>>       static void dwc3_gadget_interrupt(struct dwc3 *dwc,
>>>>>> @@ -4718,7 +4736,15 @@ void dwc3_gadget_process_pending_events(struct dwc3 *dwc)
>>>>>>     {
>>>>>>         if (dwc->pending_events) {
>>>>>>             dwc3_interrupt(dwc->irq_gadget, dwc->ev_buf);
>>>>>> +        pm_runtime_put(dwc->dev);
>>>>>
>>>>> Why the put here?
>>>>>
>>>>
>>>> To balance the get() called when setting the pending_events flag in dwc3_check_event_buf()
>>>>
>>>>       if (pm_runtime_suspended(dwc->dev)) {
>>>>           pm_runtime_get(dwc->dev);
>>>>           disable_irq_nosync(dwc->irq_gadget);
>>>>           dwc->pending_events = true;
>>>>           return IRQ_HANDLED;
>>>>       }
>>>>
>>>
>>> No this wrong. We want the device to be active from now on.
>>>
>>> runtime suspended->interrupt->pm_runtime_get->runtime_resume->process_pending_events->USB gadget resumed
>>>
>>> Only on next USB suspend you want to do the pm_runtime_put like you are doing it
>>> in dwc3_gadget_suspend_interrupt() by pm_request_autosuspend()
>>>
>>
>> That would break/block dwc3 runtime suspend during DISCONNECT case in below scenario
>>
>> runtime suspended->interrupt->pm_runtime_get (runtime usage count is 1)->runtime_resume->process_pending_events->USB gadget resumed -> USB disconnect (autosuspend blocked due to runtime usage count being 1 due to unbalanced get() ).
>>
>> The idea here is to balance the get() that was requested for processing the pending events, after processing those events. (like how we balance get() of ep_queue through put() in ep_dequeue)
>>
>> Also pm_request_autosuspend() doesnt decrement the usage count, it only requests for autosuspend.
> 
> Ah, indeed.
> 
>>
>> But I think better approach in terms of ordering is below
> 
> ok, but should dwc->pending_events be set before calling pm_runtime_get() in dwc3_check_event_buf()?

Yes that would be a better approach (just in case if there is any race 
between dwc3_check_event_buf() and the resume() path).

> Can we add a comment there that the get will be balanced out in dwc3_gadget_process_pending_events()?

Sure.
> 
>>
>> @@ -4718,7 +4736,15 @@ void dwc3_gadget_process_pending_events(struct dwc3 *dwc)
>>   {
>>       if (dwc->pending_events) {
>>           dwc3_interrupt(dwc->irq_gadget, dwc->ev_buf);
>> +        /*
>> +         * We have only stored the pending events as part
>> +         * of dwc3_interrupt() above, but those events are
>> +         * not yet handled. So explicitly invoke the
>> +         * interrupt handler for handling those events.
>> +         */
>> +        dwc3_thread_interrupt(dwc->irq_gadget, dwc->ev_buf);
>>           dwc->pending_events = false;
>>           enable_irq(dwc->irq_gadget);
>> +        pm_runtime_put(dwc->dev);
> 
> We could do the put right after dwc3_thread_interrupt().

Ack.

> 
>>
>>       }
>>   }
> 
> I think this fix should be an independent patch
> as this fixes an issue that existed prior to this series?
> also need to -cc stable?
> 
Agree. Both dwc3_thread_interrupt() and pm_runtime_put() above are 
addressing an issue that existed prior. I will submit a separate patch 
for this modification.

Thanks
Elson