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

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

[Elson Roy Serrao]

Changes in v4
 - Changed the dt property name to 'snps,runtime-suspend-on-usb-suspend' as per
   the feedback received.
 - Separated the pending events handling to a different patch as it is an
   independent fix
   https://lore.kernel.org/all/b2ab8803-d4e0-bd63-7a64-bcc411e04cfe@kernel.org/
 - Moved the bus suspend checks to runtime_checks() API to make it more relevant
   to runtime ops. Also added explicit run time check in resume common function.
 - Instead of checking the link state for bus suspend, used dwc->suspended flag
   as it directly reflects the gadget suspend/resume operations.
 
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 to detect resume events to bring the controller out of
suspend. On Qualcomm platforms the resume is handled through PHY sideband signalling.


The series is organized in below fashion:
Patch 1: This includes the modification needed from function drivers to let
UDC enter low power mode.
Patch 2: This has the modification needed in the UDC driver to trigger runtime
suspend whene a bus suspend interrupt is received. This also handles resume
and remote wakeup features from power management perspective.

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

 .../devicetree/bindings/usb/snps,dwc3.yaml    |  5 ++
 drivers/usb/dwc3/core.c                       | 28 ++++++++++-
 drivers/usb/dwc3/core.h                       |  3 ++
 drivers/usb/dwc3/gadget.c                     | 32 ++++++++++---
 drivers/usb/gadget/function/u_ether.c         | 47 +++++++++++++++----
 5 files changed, 97 insertions(+), 18 deletions(-)

-- 
2.17.1

[PATCH v4 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 v4 2/3] dt-bindings: usb: snps,dwc3: Add runtime-suspend-on-usb-suspend 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..e19a60d06d2b 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,runtime-suspend-on-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

[PATCH v4 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 dwc3 to enter a low power state 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   | 28 ++++++++++++++++++++++++++--
 drivers/usb/dwc3/core.h   |  3 +++
 drivers/usb/dwc3/gadget.c | 32 +++++++++++++++++++++++++-------
 3 files changed, 54 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 9c6bf054f15d..9bfd9bb18caf 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1518,6 +1518,9 @@ static void dwc3_get_properties(struct dwc3 *dwc)
 	dwc->dis_split_quirk = device_property_read_bool(dev,
 				"snps,dis-split-quirk");
 
+	dwc->runtime_suspend_on_usb_suspend = device_property_read_bool(dev,
+				"snps,runtime-suspend-on-usb-suspend");
+
 	dwc->lpm_nyet_threshold = lpm_nyet_threshold;
 	dwc->tx_de_emphasis = tx_de_emphasis;
 
@@ -2029,6 +2032,9 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
 
 	switch (dwc->current_dr_role) {
 	case DWC3_GCTL_PRTCAP_DEVICE:
+		/* runtime resume on bus resume scenario */
+		if (PMSG_IS_AUTO(msg) && dwc->connected)
+			break;
 		ret = dwc3_core_init_for_resume(dwc);
 		if (ret)
 			return ret;
@@ -2090,8 +2096,13 @@ static int dwc3_runtime_checks(struct dwc3 *dwc)
 {
 	switch (dwc->current_dr_role) {
 	case DWC3_GCTL_PRTCAP_DEVICE:
-		if (dwc->connected)
+		if (dwc->connected) {
+			/* bus suspend scenario */
+			if (dwc->runtime_suspend_on_usb_suspend &&
+			    dwc->suspended)
+				break;
 			return -EBUSY;
+		}
 		break;
 	case DWC3_GCTL_PRTCAP_HOST:
 	default:
@@ -2107,9 +2118,22 @@ static int dwc3_runtime_suspend(struct device *dev)
 	struct dwc3     *dwc = dev_get_drvdata(dev);
 	int		ret;
 
-	if (dwc3_runtime_checks(dwc))
+	ret = dwc3_runtime_checks(dwc);
+	if (ret)
 		return -EBUSY;
 
+	switch (dwc->current_dr_role) {
+	case DWC3_GCTL_PRTCAP_DEVICE:
+		/* bus suspend case */
+		if (!ret && dwc->connected)
+			return 0;
+		break;
+	case DWC3_GCTL_PRTCAP_HOST:
+	default:
+		/* do nothing */
+		break;
+	}
+
 	ret = dwc3_suspend_common(dwc, PMSG_AUTO_SUSPEND);
 	if (ret)
 		return ret;
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index a69ac67d89fe..f2f788a6b4b5 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -1124,6 +1124,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.
+ * @runtime_suspend_on_usb_suspend: true if dwc3 runtime suspend is allowed
+ *			during bus suspend scenario.
  */
 struct dwc3 {
 	struct work_struct	drd_work;
@@ -1340,6 +1342,7 @@ struct dwc3 {
 	int			last_fifo_depth;
 	int			num_ep_resized;
 	struct dentry		*debug_root;
+	bool			runtime_suspend_on_usb_suspend;
 };
 
 #define INCRX_BURST_MODE 0
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 5fd067151fbf..978ce0e91164 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,
-- 
2.17.1

Re: [PATCH v4 2/3] dt-bindings: usb: snps,dwc3: Add runtime-suspend-on-usb-suspend property

[Rob Herring]

On Mon, 14 Aug 2023 11:50:42 -0700, 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(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/usb/snps,dwc3.yaml: properties:snps,runtime-suspend-on-usb-suspend: 'oneOf' conditional failed, one must be fixed:
	'type' is a required property
		hint: A vendor boolean property can use "type: boolean"
	/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/usb/snps,dwc3.yaml: properties:snps,runtime-suspend-on-usb-suspend: 'oneOf' conditional failed, one must be fixed:
		'enum' is a required property
		'const' is a required property
		hint: A vendor string property with exact values has an implicit type
		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
	/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/usb/snps,dwc3.yaml: properties:snps,runtime-suspend-on-usb-suspend: 'oneOf' conditional failed, one must be fixed:
		'$ref' is a required property
		'allOf' is a required property
		hint: A vendor property needs a $ref to types.yaml
		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
	hint: Vendor specific properties must have a type and description unless they have a defined, common suffix.
	from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230814185043.9252-3-quic_eserrao@quicinc.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH v4 2/3] dt-bindings: usb: snps,dwc3: Add runtime-suspend-on-usb-suspend property

[Krzysztof Kozlowski]

On 14/08/2023 20:50, 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..e19a60d06d2b 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,runtime-suspend-on-usb-suspend:
> +    description:
> +      If True then dwc3 runtime suspend is allowed during bus suspend
> +      case even with the USB cable connected.

This was no tested... but anyway, this is no a DT property but OS
policy. There is no such thing as "runtime suspend" in the hardware,
because you describe one particular OS.

Sorry, no a DT property, drop the change entirely.


Best regards,
Krzysztof

Re: [PATCH v4 2/3] dt-bindings: usb: snps,dwc3: Add runtime-suspend-on-usb-suspend property

[Elson Serrao]

On 8/15/2023 10:44 PM, Krzysztof Kozlowski wrote:
> On 14/08/2023 20:50, 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..e19a60d06d2b 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,runtime-suspend-on-usb-suspend:
>> +    description:
>> +      If True then dwc3 runtime suspend is allowed during bus suspend
>> +      case even with the USB cable connected.
> 
> This was no tested... but anyway, this is no a DT property but OS
> policy. There is no such thing as "runtime suspend" in the hardware,
> because you describe one particular OS.
> 
> Sorry, no a DT property, drop the change entirely.
> 
> 
Hi Krzysztof

Sorry my local dt checker had some issue and it did not catch these 
errors. I have rectified it now.

This dt property is mainly for skipping dwc3 controller halt when a USB 
suspend interrupt is received with usb cable connected, so that we dont 
trigger a DISCONNECT event. Perhaps a better name would reflect the true 
usage of this?

Something like snps,skip-dwc3-halt-on-usb-suspend. dwc3 cores where 
hibernation feature is not enabled/supported can use this property

Hi Thinh,Roger
Please let me know your opinion on 'snps,skip-dwc3-halt-on-usb-suspend' 
as the quirk name.

Thanks
Elson

Re: [PATCH v4 2/3] dt-bindings: usb: snps,dwc3: Add runtime-suspend-on-usb-suspend property

[Thinh Nguyen]

On Fri, Aug 18, 2023, Elson Serrao wrote:
> 
> 
> On 8/15/2023 10:44 PM, Krzysztof Kozlowski wrote:
> > On 14/08/2023 20:50, 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..e19a60d06d2b 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,runtime-suspend-on-usb-suspend:
> > > +    description:
> > > +      If True then dwc3 runtime suspend is allowed during bus suspend
> > > +      case even with the USB cable connected.
> > 
> > This was no tested... but anyway, this is no a DT property but OS
> > policy. There is no such thing as "runtime suspend" in the hardware,
> > because you describe one particular OS.
> > 
> > Sorry, no a DT property, drop the change entirely.
> > 
> > 
> Hi Krzysztof
> 
> Sorry my local dt checker had some issue and it did not catch these errors.
> I have rectified it now.
> 
> This dt property is mainly for skipping dwc3 controller halt when a USB
> suspend interrupt is received with usb cable connected, so that we dont
> trigger a DISCONNECT event. Perhaps a better name would reflect the true
> usage of this?
> 
> Something like snps,skip-dwc3-halt-on-usb-suspend. dwc3 cores where
> hibernation feature is not enabled/supported can use this property
> 
> Hi Thinh,Roger
> Please let me know your opinion on 'snps,skip-dwc3-halt-on-usb-suspend' as
> the quirk name.
> 

I don't consider this a quirk. If there's a quirk, something's broken.
It's a behavior of the driver depending on the hardware capability.

So, this name should reflect the capability property of the phy(s).

I'm not great with naming either. Perhaps this?
snps,delegate-wakeup-interrupt

Thanks,
Thinh

Re: [PATCH v4 2/3] dt-bindings: usb: snps,dwc3: Add runtime-suspend-on-usb-suspend property

[Krzysztof Kozlowski]

On 18/08/2023 21:16, Elson Serrao wrote:
> 
> 
> On 8/15/2023 10:44 PM, Krzysztof Kozlowski wrote:
>> On 14/08/2023 20:50, 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..e19a60d06d2b 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,runtime-suspend-on-usb-suspend:
>>> +    description:
>>> +      If True then dwc3 runtime suspend is allowed during bus suspend
>>> +      case even with the USB cable connected.
>>
>> This was no tested... but anyway, this is no a DT property but OS
>> policy. There is no such thing as "runtime suspend" in the hardware,
>> because you describe one particular OS.
>>
>> Sorry, no a DT property, drop the change entirely.
>>
>>
> Hi Krzysztof
> 
> Sorry my local dt checker had some issue and it did not catch these 
> errors. I have rectified it now.
> 
> This dt property is mainly for skipping dwc3 controller halt when a USB 
> suspend interrupt is received with usb cable connected, so that we dont 
> trigger a DISCONNECT event. Perhaps a better name would reflect the true 
> usage of this?
> 
> Something like snps,skip-dwc3-halt-on-usb-suspend. dwc3 cores where 
> hibernation feature is not enabled/supported can use this property

So this is specific to DWC3 core, thus should be just implied by compatible.

Best regards,
Krzysztof

Re: [PATCH v4 2/3] dt-bindings: usb: snps,dwc3: Add runtime-suspend-on-usb-suspend property

[Elson Serrao]

On 8/19/2023 2:35 AM, Krzysztof Kozlowski wrote:
> On 18/08/2023 21:16, Elson Serrao wrote:
>>
>>
>> On 8/15/2023 10:44 PM, Krzysztof Kozlowski wrote:
>>> On 14/08/2023 20:50, 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..e19a60d06d2b 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,runtime-suspend-on-usb-suspend:
>>>> +    description:
>>>> +      If True then dwc3 runtime suspend is allowed during bus suspend
>>>> +      case even with the USB cable connected.
>>>
>>> This was no tested... but anyway, this is no a DT property but OS
>>> policy. There is no such thing as "runtime suspend" in the hardware,
>>> because you describe one particular OS.
>>>
>>> Sorry, no a DT property, drop the change entirely.
>>>
>>>
>> Hi Krzysztof
>>
>> Sorry my local dt checker had some issue and it did not catch these
>> errors. I have rectified it now.
>>
>> This dt property is mainly for skipping dwc3 controller halt when a USB
>> suspend interrupt is received with usb cable connected, so that we dont
>> trigger a DISCONNECT event. Perhaps a better name would reflect the true
>> usage of this?
>>
>> Something like snps,skip-dwc3-halt-on-usb-suspend. dwc3 cores where
>> hibernation feature is not enabled/supported can use this property
> 
> So this is specific to DWC3 core, thus should be just implied by compatible.
> 

Hi Krzysztof

Apologies for not being clear. Below is the reasoning behind this dt entry.

When bus suspend interrupt is received and if usb cable is connected, 
dwc3 driver does not suspend. The aim of this series is to handle this 
interrupt when USB cable is connected to achieve power savings. OEMs 
might have their own implementation in their glue driver to turn off 
clocks and other resources when USB is not in use, thus saving power. 
But since glue layer has dependency on dwc3 driver (parent-child 
relationship) we need to allow dwc3 driver to NOT ignore the bus suspend 
interrupt and let the dwc3 driver suspend (so that glue driver can 
suspend as well)

Now it is the responsibility of glue driver to detect USB wakeup signal 
from the host during resume (since dwc3 driver is suspended at this 
point and cannot handle interrupts). Every OEM may not have the 
capability to detect wakeup signal. The goal of this dt property is for 
the dwc3 driver to allow handling of the bus suspend interrupt when such 
a capability exists on a given HW platform. When this property is 
defined dwc3 driver knows that the low power mode entry/exit is 
controlled by the glue driver and thus it can allow the suspend 
operation when bus suspend interrupt is received.

For example on Qualcomm platforms there is a phy sideband signalling 
which detects the wakeup signal when resume is initiated by the host. 
Thus qcom platforms can benefit from this feature by defining this dt 
property. (in a parallel discussion with Thinh N to come up with a 
better name for this dt entry).

Thanks
Elson

Re: [PATCH v4 2/3] dt-bindings: usb: snps,dwc3: Add runtime-suspend-on-usb-suspend property

[Krzysztof Kozlowski]

On 23/08/2023 01:58, Elson Serrao wrote:
> 
> 
> On 8/19/2023 2:35 AM, Krzysztof Kozlowski wrote:
>> On 18/08/2023 21:16, Elson Serrao wrote:
>>>
>>>
>>> On 8/15/2023 10:44 PM, Krzysztof Kozlowski wrote:
>>>> On 14/08/2023 20:50, 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..e19a60d06d2b 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,runtime-suspend-on-usb-suspend:
>>>>> +    description:
>>>>> +      If True then dwc3 runtime suspend is allowed during bus suspend
>>>>> +      case even with the USB cable connected.
>>>>
>>>> This was no tested... but anyway, this is no a DT property but OS
>>>> policy. There is no such thing as "runtime suspend" in the hardware,
>>>> because you describe one particular OS.
>>>>
>>>> Sorry, no a DT property, drop the change entirely.
>>>>
>>>>
>>> Hi Krzysztof
>>>
>>> Sorry my local dt checker had some issue and it did not catch these
>>> errors. I have rectified it now.
>>>
>>> This dt property is mainly for skipping dwc3 controller halt when a USB
>>> suspend interrupt is received with usb cable connected, so that we dont
>>> trigger a DISCONNECT event. Perhaps a better name would reflect the true
>>> usage of this?
>>>
>>> Something like snps,skip-dwc3-halt-on-usb-suspend. dwc3 cores where
>>> hibernation feature is not enabled/supported can use this property
>>
>> So this is specific to DWC3 core, thus should be just implied by compatible.
>>
> 
> Hi Krzysztof
> 
> Apologies for not being clear. Below is the reasoning behind this dt entry.
> 
> When bus suspend interrupt is received and if usb cable is connected, 
> dwc3 driver does not suspend. The aim of this series is to handle this 
> interrupt when USB cable is connected to achieve power savings. OEMs 
> might have their own implementation in their glue driver to turn off 
> clocks and other resources when USB is not in use, thus saving power. 
> But since glue layer has dependency on dwc3 driver (parent-child 
> relationship) we need to allow dwc3 driver to NOT ignore the bus suspend 
> interrupt and let the dwc3 driver suspend (so that glue driver can 
> suspend as well)

All this describes current OS implementation so has nothing to do with
bindings.

> 
> Now it is the responsibility of glue driver to detect USB wakeup signal 
> from the host during resume (since dwc3 driver is suspended at this 
> point and cannot handle interrupts). Every OEM may not have the 
> capability to detect wakeup signal. 

Again, driver architecture.

> The goal of this dt property is for 
> the dwc3 driver to allow handling of the bus suspend interrupt when such 

DT properties are not "for the drivers".

> a capability exists on a given HW platform. When this property is

Each platform has this capability. If not, then it is
compatible-related, as I said before which you did not address.


> defined dwc3 driver knows that the low power mode entry/exit is 
> controlled by the glue driver and thus it can allow the suspend 
> operation when bus suspend interrupt is received.
> 
> For example on Qualcomm platforms there is a phy sideband signalling 
> which detects the wakeup signal when resume is initiated by the host.

So compatible-specific.

> Thus qcom platforms can benefit from this feature by defining this dt 
> property. (in a parallel discussion with Thinh N to come up with a 
> better name for this dt entry).

Thanks, with quite a long message you at the end admitted this is
compatible-specific. Exactly what I wrote it one sentence previously.

Best regards,
Krzysztof

Re: [PATCH v4 2/3] dt-bindings: usb: snps,dwc3: Add runtime-suspend-on-usb-suspend property

[Roger Quadros]

On 23/08/2023 09:34, Krzysztof Kozlowski wrote:
> On 23/08/2023 01:58, Elson Serrao wrote:
>>
>>
>> On 8/19/2023 2:35 AM, Krzysztof Kozlowski wrote:
>>> On 18/08/2023 21:16, Elson Serrao wrote:
>>>>
>>>>
>>>> On 8/15/2023 10:44 PM, Krzysztof Kozlowski wrote:
>>>>> On 14/08/2023 20:50, 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..e19a60d06d2b 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,runtime-suspend-on-usb-suspend:
>>>>>> +    description:
>>>>>> +      If True then dwc3 runtime suspend is allowed during bus suspend
>>>>>> +      case even with the USB cable connected.
>>>>>
>>>>> This was no tested... but anyway, this is no a DT property but OS
>>>>> policy. There is no such thing as "runtime suspend" in the hardware,
>>>>> because you describe one particular OS.
>>>>>
>>>>> Sorry, no a DT property, drop the change entirely.
>>>>>
>>>>>
>>>> Hi Krzysztof
>>>>
>>>> Sorry my local dt checker had some issue and it did not catch these
>>>> errors. I have rectified it now.
>>>>
>>>> This dt property is mainly for skipping dwc3 controller halt when a USB
>>>> suspend interrupt is received with usb cable connected, so that we dont
>>>> trigger a DISCONNECT event. Perhaps a better name would reflect the true
>>>> usage of this?
>>>>
>>>> Something like snps,skip-dwc3-halt-on-usb-suspend. dwc3 cores where
>>>> hibernation feature is not enabled/supported can use this property
>>>
>>> So this is specific to DWC3 core, thus should be just implied by compatible.
>>>
>>
>> Hi Krzysztof
>>
>> Apologies for not being clear. Below is the reasoning behind this dt entry.
>>
>> When bus suspend interrupt is received and if usb cable is connected, 
>> dwc3 driver does not suspend. The aim of this series is to handle this 
>> interrupt when USB cable is connected to achieve power savings. OEMs 
>> might have their own implementation in their glue driver to turn off 
>> clocks and other resources when USB is not in use, thus saving power. 
>> But since glue layer has dependency on dwc3 driver (parent-child 
>> relationship) we need to allow dwc3 driver to NOT ignore the bus suspend 
>> interrupt and let the dwc3 driver suspend (so that glue driver can 
>> suspend as well)
> 
> All this describes current OS implementation so has nothing to do with
> bindings.
> 
>>
>> Now it is the responsibility of glue driver to detect USB wakeup signal 
>> from the host during resume (since dwc3 driver is suspended at this 
>> point and cannot handle interrupts). Every OEM may not have the 
>> capability to detect wakeup signal. 
> 
> Again, driver architecture.

This is not driver architecture but SoC (hardware) architecture.
Most SoCs have some kind of Wrapper h/w logic around the USB h/w module
where they implement such logic like power/clocking/wake-up handling etc.
So this information (whether wake-up is supported or not)
should be already known to the respective Wrapper driver.

Now the missing part is how to convey this information to the USB (DWC3)
driver so it behaves in the correct way.

> 
>> The goal of this dt property is for 
>> the dwc3 driver to allow handling of the bus suspend interrupt when such 
> 
> DT properties are not "for the drivers".
> 
>> a capability exists on a given HW platform. When this property is
> 
> Each platform has this capability. If not, then it is
> compatible-related, as I said before which you did not address.
> 
> 
>> defined dwc3 driver knows that the low power mode entry/exit is 
>> controlled by the glue driver and thus it can allow the suspend 
>> operation when bus suspend interrupt is received.
>>
>> For example on Qualcomm platforms there is a phy sideband signalling 
>> which detects the wakeup signal when resume is initiated by the host.
> 
> So compatible-specific.
> 
>> Thus qcom platforms can benefit from this feature by defining this dt 
>> property. (in a parallel discussion with Thinh N to come up with a 
>> better name for this dt entry).
> 
> Thanks, with quite a long message you at the end admitted this is
> compatible-specific. Exactly what I wrote it one sentence previously.
> 
> Best regards,
> Krzysztof
> 

-- 
cheers,
-roger

Re: [PATCH v4 2/3] dt-bindings: usb: snps,dwc3: Add runtime-suspend-on-usb-suspend property

[Thinh Nguyen]

On Wed, Aug 23, 2023, Roger Quadros wrote:
> 
> 
> On 23/08/2023 09:34, Krzysztof Kozlowski wrote:
> > On 23/08/2023 01:58, Elson Serrao wrote:
> >>
> >>
> >> On 8/19/2023 2:35 AM, Krzysztof Kozlowski wrote:
> >>> On 18/08/2023 21:16, Elson Serrao wrote:
> >>>>
> >>>>
> >>>> On 8/15/2023 10:44 PM, Krzysztof Kozlowski wrote:
> >>>>> On 14/08/2023 20:50, 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..e19a60d06d2b 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,runtime-suspend-on-usb-suspend:
> >>>>>> +    description:
> >>>>>> +      If True then dwc3 runtime suspend is allowed during bus suspend
> >>>>>> +      case even with the USB cable connected.
> >>>>>
> >>>>> This was no tested... but anyway, this is no a DT property but OS
> >>>>> policy. There is no such thing as "runtime suspend" in the hardware,
> >>>>> because you describe one particular OS.
> >>>>>
> >>>>> Sorry, no a DT property, drop the change entirely.
> >>>>>
> >>>>>
> >>>> Hi Krzysztof
> >>>>
> >>>> Sorry my local dt checker had some issue and it did not catch these
> >>>> errors. I have rectified it now.
> >>>>
> >>>> This dt property is mainly for skipping dwc3 controller halt when a USB
> >>>> suspend interrupt is received with usb cable connected, so that we dont
> >>>> trigger a DISCONNECT event. Perhaps a better name would reflect the true
> >>>> usage of this?
> >>>>
> >>>> Something like snps,skip-dwc3-halt-on-usb-suspend. dwc3 cores where
> >>>> hibernation feature is not enabled/supported can use this property
> >>>
> >>> So this is specific to DWC3 core, thus should be just implied by compatible.
> >>>
> >>
> >> Hi Krzysztof
> >>
> >> Apologies for not being clear. Below is the reasoning behind this dt entry.
> >>
> >> When bus suspend interrupt is received and if usb cable is connected, 
> >> dwc3 driver does not suspend. The aim of this series is to handle this 
> >> interrupt when USB cable is connected to achieve power savings. OEMs 
> >> might have their own implementation in their glue driver to turn off 
> >> clocks and other resources when USB is not in use, thus saving power. 
> >> But since glue layer has dependency on dwc3 driver (parent-child 
> >> relationship) we need to allow dwc3 driver to NOT ignore the bus suspend 
> >> interrupt and let the dwc3 driver suspend (so that glue driver can 
> >> suspend as well)
> > 
> > All this describes current OS implementation so has nothing to do with
> > bindings.
> > 
> >>
> >> Now it is the responsibility of glue driver to detect USB wakeup signal 
> >> from the host during resume (since dwc3 driver is suspended at this 
> >> point and cannot handle interrupts). Every OEM may not have the 
> >> capability to detect wakeup signal. 
> > 
> > Again, driver architecture.
> 
> This is not driver architecture but SoC (hardware) architecture.
> Most SoCs have some kind of Wrapper h/w logic around the USB h/w module
> where they implement such logic like power/clocking/wake-up handling etc.
> So this information (whether wake-up is supported or not)
> should be already known to the respective Wrapper driver.
> 
> Now the missing part is how to convey this information to the USB (DWC3)
> driver so it behaves in the correct way.
> 
> > 
> >> The goal of this dt property is for 
> >> the dwc3 driver to allow handling of the bus suspend interrupt when such 
> > 
> > DT properties are not "for the drivers".
> > 
> >> a capability exists on a given HW platform. When this property is
> > 
> > Each platform has this capability. If not, then it is
> > compatible-related, as I said before which you did not address.
> > 
> > 
> >> defined dwc3 driver knows that the low power mode entry/exit is 
> >> controlled by the glue driver and thus it can allow the suspend 
> >> operation when bus suspend interrupt is received.
> >>
> >> For example on Qualcomm platforms there is a phy sideband signalling 
> >> which detects the wakeup signal when resume is initiated by the host.
> > 
> > So compatible-specific.
> > 
> >> Thus qcom platforms can benefit from this feature by defining this dt 
> >> property. (in a parallel discussion with Thinh N to come up with a 
> >> better name for this dt entry).
> > 
> > Thanks, with quite a long message you at the end admitted this is
> > compatible-specific. Exactly what I wrote it one sentence previously.
> > 

Various dwc3 platforms often share a common capability that can be
shared with a common dt property. If we dedicate a property such as in
this case, it helps designers enable a certain feature without updating
the driver every time a new platform is introduced. It also helps keep
the driver a bit simpler on the compatible checks.

Regardless, what Krzysztof said is valid. Perhaps we can look into
enhancing dwc3 to maintain shared behavior based on compatible instead?

Thanks,
Thinh

Re: [PATCH v4 2/3] dt-bindings: usb: snps,dwc3: Add runtime-suspend-on-usb-suspend property

[Krzysztof Kozlowski]

On 26/08/2023 03:53, Thinh Nguyen wrote:
>>>> For example on Qualcomm platforms there is a phy sideband signalling 
>>>> which detects the wakeup signal when resume is initiated by the host.
>>>
>>> So compatible-specific.
>>>
>>>> Thus qcom platforms can benefit from this feature by defining this dt 
>>>> property. (in a parallel discussion with Thinh N to come up with a 
>>>> better name for this dt entry).
>>>
>>> Thanks, with quite a long message you at the end admitted this is
>>> compatible-specific. Exactly what I wrote it one sentence previously.
>>>
> 
> Various dwc3 platforms often share a common capability that can be
> shared with a common dt property. If we dedicate a property such as in
> this case, it helps designers enable a certain feature without updating
> the driver every time a new platform is introduced. It also helps keep
> the driver a bit simpler on the compatible checks.

That's not the purpose of bindings. Sorry.

> 
> Regardless, what Krzysztof said is valid. Perhaps we can look into
> enhancing dwc3 to maintain shared behavior based on compatible instead?


Best regards,
Krzysztof

Re: [PATCH v4 2/3] dt-bindings: usb: snps,dwc3: Add runtime-suspend-on-usb-suspend property

[Elson Serrao]

On 8/26/2023 1:39 AM, Krzysztof Kozlowski wrote:
> On 26/08/2023 03:53, Thinh Nguyen wrote:
>>>>> For example on Qualcomm platforms there is a phy sideband signalling
>>>>> which detects the wakeup signal when resume is initiated by the host.
>>>>
>>>> So compatible-specific.
>>>>
>>>>> Thus qcom platforms can benefit from this feature by defining this dt
>>>>> property. (in a parallel discussion with Thinh N to come up with a
>>>>> better name for this dt entry).
>>>>
>>>> Thanks, with quite a long message you at the end admitted this is
>>>> compatible-specific. Exactly what I wrote it one sentence previously.
>>>>
>>
>> Various dwc3 platforms often share a common capability that can be
>> shared with a common dt property. If we dedicate a property such as in
>> this case, it helps designers enable a certain feature without updating
>> the driver every time a new platform is introduced. It also helps keep
>> the driver a bit simpler on the compatible checks.
> 
> That's not the purpose of bindings. Sorry.
> 
>>
>> Regardless, what Krzysztof said is valid. Perhaps we can look into
>> enhancing dwc3 to maintain shared behavior based on compatible instead?
> 
> 
Thank you Krzysztof, Thinh, Roger for your comments and feedback. I will 
upload v5 making this change compatible-specifc.

Best Regards
Elson

Re: [PATCH v4 2/3] dt-bindings: usb: snps,dwc3: Add runtime-suspend-on-usb-suspend property

[Thinh Nguyen]

On Mon, Aug 28, 2023, Elson Serrao wrote:
> 
> 
> On 8/26/2023 1:39 AM, Krzysztof Kozlowski wrote:
> > On 26/08/2023 03:53, Thinh Nguyen wrote:
> > > > > > For example on Qualcomm platforms there is a phy sideband signalling
> > > > > > which detects the wakeup signal when resume is initiated by the host.
> > > > > 
> > > > > So compatible-specific.
> > > > > 
> > > > > > Thus qcom platforms can benefit from this feature by defining this dt
> > > > > > property. (in a parallel discussion with Thinh N to come up with a
> > > > > > better name for this dt entry).
> > > > > 
> > > > > Thanks, with quite a long message you at the end admitted this is
> > > > > compatible-specific. Exactly what I wrote it one sentence previously.
> > > > > 
> > > 
> > > Various dwc3 platforms often share a common capability that can be
> > > shared with a common dt property. If we dedicate a property such as in
> > > this case, it helps designers enable a certain feature without updating
> > > the driver every time a new platform is introduced. It also helps keep
> > > the driver a bit simpler on the compatible checks.
> > 
> > That's not the purpose of bindings. Sorry.
> > 
> > > 
> > > Regardless, what Krzysztof said is valid. Perhaps we can look into
> > > enhancing dwc3 to maintain shared behavior based on compatible instead?
> > 
> > 
> Thank you Krzysztof, Thinh, Roger for your comments and feedback. I will
> upload v5 making this change compatible-specifc.
> 

Hi Elson,

Just want to clarify, there are dwc3 properties and there are dt binding
properties. Often the case that dt binding matches 1-to-1 with dwc3
driver property. Now, we need to enhance the checkers so that the dwc3
driver property to match cases where it is platform specific and through
compatible string.

Thanks,
Thinh

Re: [PATCH v4 2/3] dt-bindings: usb: snps,dwc3: Add runtime-suspend-on-usb-suspend property

[Elson Serrao]

On 8/29/2023 6:37 PM, Thinh Nguyen wrote:
> Just want to clarify, there are dwc3 properties and there are dt binding
> properties. Often the case that dt binding matches 1-to-1 with dwc3
> driver property. Now, we need to enhance the checkers so that the dwc3
> driver property to match cases where it is platform specific and through
> compatible string.
> 

Thank you for the clarification Thinh.
To confirm, we would need to modify the driver to parse a new compatible 
string (say "snps,dwc3-ext-wakeup") and add .data field so that the 
driver is aware that this particular platform supports external wakeup 
detection.Right ?

Regards
Elson

Re: [PATCH v4 2/3] dt-bindings: usb: snps,dwc3: Add runtime-suspend-on-usb-suspend property

[Krzysztof Kozlowski]

On 30/08/2023 06:31, Elson Serrao wrote:
> 
> 
> On 8/29/2023 6:37 PM, Thinh Nguyen wrote:
>> Just want to clarify, there are dwc3 properties and there are dt binding
>> properties. Often the case that dt binding matches 1-to-1 with dwc3
>> driver property. Now, we need to enhance the checkers so that the dwc3
>> driver property to match cases where it is platform specific and through
>> compatible string.
>>
> 
> Thank you for the clarification Thinh.
> To confirm, we would need to modify the driver to parse a new compatible 
> string (say "snps,dwc3-ext-wakeup") and add .data field so that the 
> driver is aware that this particular platform supports external wakeup 
> detection.Right ?

No, it's not then platform specific. You said it depends on each
platform. Platform is Qualcomm SM8450 for example.

Best regards,
Krzysztof

Re: [PATCH v4 2/3] dt-bindings: usb: snps,dwc3: Add runtime-suspend-on-usb-suspend property

[Thinh Nguyen]

On Wed, Aug 30, 2023, Krzysztof Kozlowski wrote:
> On 30/08/2023 06:31, Elson Serrao wrote:
> > 
> > 
> > On 8/29/2023 6:37 PM, Thinh Nguyen wrote:
> >> Just want to clarify, there are dwc3 properties and there are dt binding
> >> properties. Often the case that dt binding matches 1-to-1 with dwc3
> >> driver property. Now, we need to enhance the checkers so that the dwc3
> >> driver property to match cases where it is platform specific and through
> >> compatible string.
> >>
> > 
> > Thank you for the clarification Thinh.
> > To confirm, we would need to modify the driver to parse a new compatible 
> > string (say "snps,dwc3-ext-wakeup") and add .data field so that the 
> > driver is aware that this particular platform supports external wakeup 
> > detection.Right ?
> 
> No, it's not then platform specific. You said it depends on each
> platform. Platform is Qualcomm SM8450 for example.
> 

Hi Elson,

Use the compatible string of your platform.

e.g.
if (dev->of_node) {
	struct device_node *parent = of_get_parent(dev->of_node);

	dwc->no_disconnect_on_usb_suspend =
		of_device_is_compatible(parent, "qcom,your-compatible-string") ||
		of_device_is_compatible(parent, "some-other-platform");
}

You need to enhance dwc3_get_properties(). This may get big as dwc3 adds
more properties. Perhaps you can help come up with ideas to keep this
clean. Perhaps we can separate this out of dwc3 core.c?

Thanks,
Thinh

Re: [PATCH v4 2/3] dt-bindings: usb: snps,dwc3: Add runtime-suspend-on-usb-suspend property

[Krzysztof Kozlowski]

On 31/08/2023 05:01, Thinh Nguyen wrote:
> On Wed, Aug 30, 2023, Krzysztof Kozlowski wrote:
>> On 30/08/2023 06:31, Elson Serrao wrote:
>>>
>>>
>>> On 8/29/2023 6:37 PM, Thinh Nguyen wrote:
>>>> Just want to clarify, there are dwc3 properties and there are dt binding
>>>> properties. Often the case that dt binding matches 1-to-1 with dwc3
>>>> driver property. Now, we need to enhance the checkers so that the dwc3
>>>> driver property to match cases where it is platform specific and through
>>>> compatible string.
>>>>
>>>
>>> Thank you for the clarification Thinh.
>>> To confirm, we would need to modify the driver to parse a new compatible 
>>> string (say "snps,dwc3-ext-wakeup") and add .data field so that the 
>>> driver is aware that this particular platform supports external wakeup 
>>> detection.Right ?
>>
>> No, it's not then platform specific. You said it depends on each
>> platform. Platform is Qualcomm SM8450 for example.
>>
> 
> Hi Elson,
> 
> Use the compatible string of your platform.
> 
> e.g.
> if (dev->of_node) {
> 	struct device_node *parent = of_get_parent(dev->of_node);
> 
> 	dwc->no_disconnect_on_usb_suspend =
> 		of_device_is_compatible(parent, "qcom,your-compatible-string") ||
> 		of_device_is_compatible(parent, "some-other-platform");
> }
> 
> You need to enhance dwc3_get_properties(). This may get big as dwc3 adds
> more properties. Perhaps you can help come up with ideas to keep this
> clean. Perhaps we can separate this out of dwc3 core.c?

This should be a flag or quirk in device ID table match data.

Best regards,
Krzysztof

Re: [PATCH v4 2/3] dt-bindings: usb: snps,dwc3: Add runtime-suspend-on-usb-suspend property

[Elson Serrao]

On 8/30/2023 11:29 PM, Krzysztof Kozlowski wrote:
> On 31/08/2023 05:01, Thinh Nguyen wrote:
>> On Wed, Aug 30, 2023, Krzysztof Kozlowski wrote:
>>> On 30/08/2023 06:31, Elson Serrao wrote:
>>>>
>>>>
>>>> On 8/29/2023 6:37 PM, Thinh Nguyen wrote:
>>>>> Just want to clarify, there are dwc3 properties and there are dt binding
>>>>> properties. Often the case that dt binding matches 1-to-1 with dwc3
>>>>> driver property. Now, we need to enhance the checkers so that the dwc3
>>>>> driver property to match cases where it is platform specific and through
>>>>> compatible string.
>>>>>
>>>>
>>>> Thank you for the clarification Thinh.
>>>> To confirm, we would need to modify the driver to parse a new compatible
>>>> string (say "snps,dwc3-ext-wakeup") and add .data field so that the
>>>> driver is aware that this particular platform supports external wakeup
>>>> detection.Right ?
>>>
>>> No, it's not then platform specific. You said it depends on each
>>> platform. Platform is Qualcomm SM8450 for example.
>>>
>>
>> Hi Elson,
>>
>> Use the compatible string of your platform.
>>
>> e.g.
>> if (dev->of_node) {
>> 	struct device_node *parent = of_get_parent(dev->of_node);
>>
>> 	dwc->no_disconnect_on_usb_suspend =
>> 		of_device_is_compatible(parent, "qcom,your-compatible-string") ||
>> 		of_device_is_compatible(parent, "some-other-platform");
>> }
>>
>> You need to enhance dwc3_get_properties(). This may get big as dwc3 adds
>> more properties. Perhaps you can help come up with ideas to keep this
>> clean. Perhaps we can separate this out of dwc3 core.c?

HI Thinh

Apologies for the delayed response.
Series 
https://patchwork.kernel.org/project/linux-usb/cover/1655094654-24052-1-git-send-email-quic_kriskura@quicinc.com/ 
from Krishna K, introduced a dt property 'wakeup-source' which indicates 
a platforms capability to handle wakeup interrupts. Based on this 
property, glue drivers can inform dwc3 core that the device is wakeup 
capable through device_init_wakeup(). For example dwc3-qcom driver 
informs it like below as per the implementation done in the above series

	wakeup_source = of_property_read_bool(dev->of_node, "wakeup-source");
	device_init_wakeup(&pdev->dev, wakeup_source);
	device_init_wakeup(&qcom->dwc3->dev, wakeup_source);

The dwc3 core now can access this info through 
device_may_wakeup(dwc->dev) while checking for bus suspend scenario to 
know whether the platform is capable of detecting wakeup.

Please let me know your thoughts on this approach.

Thanks
Elson

Re: [PATCH v4 2/3] dt-bindings: usb: snps,dwc3: Add runtime-suspend-on-usb-suspend property

[Thinh Nguyen]

On Thu, Sep 21, 2023, Elson Serrao wrote:
> 
> 
> On 8/30/2023 11:29 PM, Krzysztof Kozlowski wrote:
> > On 31/08/2023 05:01, Thinh Nguyen wrote:
> > > On Wed, Aug 30, 2023, Krzysztof Kozlowski wrote:
> > > > On 30/08/2023 06:31, Elson Serrao wrote:
> > > > > 
> > > > > 
> > > > > On 8/29/2023 6:37 PM, Thinh Nguyen wrote:
> > > > > > Just want to clarify, there are dwc3 properties and there are dt binding
> > > > > > properties. Often the case that dt binding matches 1-to-1 with dwc3
> > > > > > driver property. Now, we need to enhance the checkers so that the dwc3
> > > > > > driver property to match cases where it is platform specific and through
> > > > > > compatible string.
> > > > > > 
> > > > > 
> > > > > Thank you for the clarification Thinh.
> > > > > To confirm, we would need to modify the driver to parse a new compatible
> > > > > string (say "snps,dwc3-ext-wakeup") and add .data field so that the
> > > > > driver is aware that this particular platform supports external wakeup
> > > > > detection.Right ?
> > > > 
> > > > No, it's not then platform specific. You said it depends on each
> > > > platform. Platform is Qualcomm SM8450 for example.
> > > > 
> > > 
> > > Hi Elson,
> > > 
> > > Use the compatible string of your platform.
> > > 
> > > e.g.
> > > if (dev->of_node) {
> > > 	struct device_node *parent = of_get_parent(dev->of_node);
> > > 
> > > 	dwc->no_disconnect_on_usb_suspend =
> > > 		of_device_is_compatible(parent, "qcom,your-compatible-string") ||
> > > 		of_device_is_compatible(parent, "some-other-platform");
> > > }
> > > 
> > > You need to enhance dwc3_get_properties(). This may get big as dwc3 adds
> > > more properties. Perhaps you can help come up with ideas to keep this
> > > clean. Perhaps we can separate this out of dwc3 core.c?
> 
> HI Thinh
> 
> Apologies for the delayed response.
> Series https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-usb/cover/1655094654-24052-1-git-send-email-quic_kriskura@quicinc.com/__;!!A4F2R9G_pg!YGlVy7No98zfEM-X5iWRhIUJ-gJEJn_gbTR4k12avzENV1TXf7cwJLZUezYzAU-rnHIbbqA1UWM0IE0R-t5SMMTJLwLZ$
> from Krishna K, introduced a dt property 'wakeup-source' which indicates a
> platforms capability to handle wakeup interrupts. Based on this property,
> glue drivers can inform dwc3 core that the device is wakeup capable through
> device_init_wakeup(). For example dwc3-qcom driver informs it like below as
> per the implementation done in the above series
> 
> 	wakeup_source = of_property_read_bool(dev->of_node, "wakeup-source");
> 	device_init_wakeup(&pdev->dev, wakeup_source);
> 	device_init_wakeup(&qcom->dwc3->dev, wakeup_source);
> 
> The dwc3 core now can access this info through device_may_wakeup(dwc->dev)
> while checking for bus suspend scenario to know whether the platform is
> capable of detecting wakeup.
> 
> Please let me know your thoughts on this approach.
> 

Hi Elson,

I think that it may not work for everyone. Some platforms may indicate
wakeup-source but should only be applicable in selected scenarios.
(e.g. Roger's platform was only intended to keep connect on suspend)

Also, how will you disable it for certain platforms? Probably will need
to use compatible string then too.

Thanks,
Thinh

Re: [PATCH v4 2/3] dt-bindings: usb: snps,dwc3: Add runtime-suspend-on-usb-suspend property

[Elson Serrao]

>> HI Thinh
>>
>> Apologies for the delayed response.
>> Series https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-usb/cover/1655094654-24052-1-git-send-email-quic_kriskura@quicinc.com/__;!!A4F2R9G_pg!YGlVy7No98zfEM-X5iWRhIUJ-gJEJn_gbTR4k12avzENV1TXf7cwJLZUezYzAU-rnHIbbqA1UWM0IE0R-t5SMMTJLwLZ$
>> from Krishna K, introduced a dt property 'wakeup-source' which indicates a
>> platforms capability to handle wakeup interrupts. Based on this property,
>> glue drivers can inform dwc3 core that the device is wakeup capable through
>> device_init_wakeup(). For example dwc3-qcom driver informs it like below as
>> per the implementation done in the above series
>>
>> 	wakeup_source = of_property_read_bool(dev->of_node, "wakeup-source");
>> 	device_init_wakeup(&pdev->dev, wakeup_source);
>> 	device_init_wakeup(&qcom->dwc3->dev, wakeup_source);
>>
>> The dwc3 core now can access this info through device_may_wakeup(dwc->dev)
>> while checking for bus suspend scenario to know whether the platform is
>> capable of detecting wakeup.
>>
>> Please let me know your thoughts on this approach.
>>
> 
> Hi Elson,
> 
> I think that it may not work for everyone. Some platforms may indicate
> wakeup-source but should only be applicable in selected scenarios.
> (e.g. Roger's platform was only intended to keep connect on suspend)
> 
> Also, how will you disable it for certain platforms? Probably will need
> to use compatible string then too.
> 

Hi Thinh

Thank you for your feedback. As an alternative approach, how about 
exposing an API from dwc3 core that glue drivers can call to enable 
runtime suspend during bus suspend feature ( i.e this API sets 
dwc->runtime_suspend_on_usb_suspend field).

Only the platforms that need this feature to be enabled, can call this 
API after the child (dwc3 core) probe.

Thanks
Elson

Re: [PATCH v4 2/3] dt-bindings: usb: snps,dwc3: Add runtime-suspend-on-usb-suspend property

[Thinh Nguyen]

On Tue, Oct 17, 2023, Elson Serrao wrote:
> 
> 
> > > HI Thinh
> > > 
> > > Apologies for the delayed response.
> > > Series https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-usb/cover/1655094654-24052-1-git-send-email-quic_kriskura@quicinc.com/__;!!A4F2R9G_pg!YGlVy7No98zfEM-X5iWRhIUJ-gJEJn_gbTR4k12avzENV1TXf7cwJLZUezYzAU-rnHIbbqA1UWM0IE0R-t5SMMTJLwLZ$
> > > from Krishna K, introduced a dt property 'wakeup-source' which indicates a
> > > platforms capability to handle wakeup interrupts. Based on this property,
> > > glue drivers can inform dwc3 core that the device is wakeup capable through
> > > device_init_wakeup(). For example dwc3-qcom driver informs it like below as
> > > per the implementation done in the above series
> > > 
> > > 	wakeup_source = of_property_read_bool(dev->of_node, "wakeup-source");
> > > 	device_init_wakeup(&pdev->dev, wakeup_source);
> > > 	device_init_wakeup(&qcom->dwc3->dev, wakeup_source);
> > > 
> > > The dwc3 core now can access this info through device_may_wakeup(dwc->dev)
> > > while checking for bus suspend scenario to know whether the platform is
> > > capable of detecting wakeup.
> > > 
> > > Please let me know your thoughts on this approach.
> > > 
> > 
> > Hi Elson,
> > 
> > I think that it may not work for everyone. Some platforms may indicate
> > wakeup-source but should only be applicable in selected scenarios.
> > (e.g. Roger's platform was only intended to keep connect on suspend)
> > 
> > Also, how will you disable it for certain platforms? Probably will need
> > to use compatible string then too.
> > 
> 
> Hi Thinh
> 
> Thank you for your feedback. As an alternative approach, how about exposing
> an API from dwc3 core that glue drivers can call to enable runtime suspend
> during bus suspend feature ( i.e this API sets
> dwc->runtime_suspend_on_usb_suspend field).
> 
> Only the platforms that need this feature to be enabled, can call this API
> after the child (dwc3 core) probe.
> 

You mean after Bjorn's updates? That sounds good to me. Please make it
generic so that we can set other driver properties too.

Thanks,
Thinh

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

[Roger Quadros]

Hi Elson,

On 14/08/2023 21:50, Elson Roy Serrao wrote:
> The current implementation blocks the runtime pm operations when cable
> is connected. This would block dwc3 to enter a low power state 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   | 28 ++++++++++++++++++++++++++--
>  drivers/usb/dwc3/core.h   |  3 +++
>  drivers/usb/dwc3/gadget.c | 32 +++++++++++++++++++++++++-------
>  3 files changed, 54 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 9c6bf054f15d..9bfd9bb18caf 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1518,6 +1518,9 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>  	dwc->dis_split_quirk = device_property_read_bool(dev,
>  				"snps,dis-split-quirk");
>  
> +	dwc->runtime_suspend_on_usb_suspend = device_property_read_bool(dev,
> +				"snps,runtime-suspend-on-usb-suspend");
> +
>  	dwc->lpm_nyet_threshold = lpm_nyet_threshold;
>  	dwc->tx_de_emphasis = tx_de_emphasis;
>  
> @@ -2029,6 +2032,9 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
>  
>  	switch (dwc->current_dr_role) {
>  	case DWC3_GCTL_PRTCAP_DEVICE:
> +		/* runtime resume on bus resume scenario */
> +		if (PMSG_IS_AUTO(msg) && dwc->connected)
> +			break;
>  		ret = dwc3_core_init_for_resume(dwc);
>  		if (ret)
>  			return ret;
> @@ -2090,8 +2096,13 @@ static int dwc3_runtime_checks(struct dwc3 *dwc)
>  {
>  	switch (dwc->current_dr_role) {
>  	case DWC3_GCTL_PRTCAP_DEVICE:
> -		if (dwc->connected)
> +		if (dwc->connected) {
> +			/* bus suspend scenario */
> +			if (dwc->runtime_suspend_on_usb_suspend &&
> +			    dwc->suspended)

If dwc is already suspended why do we return -EBUSY?
Should this be !dwc->suspended?

> +				break;
>  			return -EBUSY;
> +		}
>  		break;
>  	case DWC3_GCTL_PRTCAP_HOST:
>  	default:
> @@ -2107,9 +2118,22 @@ static int dwc3_runtime_suspend(struct device *dev)
>  	struct dwc3     *dwc = dev_get_drvdata(dev);
>  	int		ret;
>  
> -	if (dwc3_runtime_checks(dwc))
> +	ret = dwc3_runtime_checks(dwc);
> +	if (ret)
>  		return -EBUSY;
>  
> +	switch (dwc->current_dr_role) {
> +	case DWC3_GCTL_PRTCAP_DEVICE:
> +		/* bus suspend case */
> +		if (!ret && dwc->connected)

No need to check !ret again as it will never happen because
we are returning -EBUSY earlier if (ret);

> +			return 0;
> +		break;
> +	case DWC3_GCTL_PRTCAP_HOST:
> +	default:
> +		/* do nothing */
> +		break;
> +	}
> +

While this takes care of runtime suspend case, what about system_suspend?
Should this check be moved to dwc3_suspend_common() instead?

>  	ret = dwc3_suspend_common(dwc, PMSG_AUTO_SUSPEND);
>  	if (ret)
>  		return ret;
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index a69ac67d89fe..f2f788a6b4b5 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -1124,6 +1124,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.
> + * @runtime_suspend_on_usb_suspend: true if dwc3 runtime suspend is allowed
> + *			during bus suspend scenario.
>   */
>  struct dwc3 {
>  	struct work_struct	drd_work;
> @@ -1340,6 +1342,7 @@ struct dwc3 {
>  	int			last_fifo_depth;
>  	int			num_ep_resized;
>  	struct dentry		*debug_root;
> +	bool			runtime_suspend_on_usb_suspend;
>  };
>  
>  #define INCRX_BURST_MODE 0
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 5fd067151fbf..978ce0e91164 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,

-- 
cheers,
-roger

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

[Elson Serrao]

On 10/24/2023 3:14 AM, Roger Quadros wrote:
> Hi Elson,
> 
> On 14/08/2023 21:50, Elson Roy Serrao wrote:
>> The current implementation blocks the runtime pm operations when cable
>> is connected. This would block dwc3 to enter a low power state 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   | 28 ++++++++++++++++++++++++++--
>>   drivers/usb/dwc3/core.h   |  3 +++
>>   drivers/usb/dwc3/gadget.c | 32 +++++++++++++++++++++++++-------
>>   3 files changed, 54 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index 9c6bf054f15d..9bfd9bb18caf 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -1518,6 +1518,9 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>>   	dwc->dis_split_quirk = device_property_read_bool(dev,
>>   				"snps,dis-split-quirk");
>>   
>> +	dwc->runtime_suspend_on_usb_suspend = device_property_read_bool(dev,
>> +				"snps,runtime-suspend-on-usb-suspend");
>> +
>>   	dwc->lpm_nyet_threshold = lpm_nyet_threshold;
>>   	dwc->tx_de_emphasis = tx_de_emphasis;
>>   
>> @@ -2029,6 +2032,9 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
>>   
>>   	switch (dwc->current_dr_role) {
>>   	case DWC3_GCTL_PRTCAP_DEVICE:
>> +		/* runtime resume on bus resume scenario */
>> +		if (PMSG_IS_AUTO(msg) && dwc->connected)
>> +			break;
>>   		ret = dwc3_core_init_for_resume(dwc);
>>   		if (ret)
>>   			return ret;
>> @@ -2090,8 +2096,13 @@ static int dwc3_runtime_checks(struct dwc3 *dwc)
>>   {
>>   	switch (dwc->current_dr_role) {
>>   	case DWC3_GCTL_PRTCAP_DEVICE:
>> -		if (dwc->connected)
>> +		if (dwc->connected) {
>> +			/* bus suspend scenario */
>> +			if (dwc->runtime_suspend_on_usb_suspend &&
>> +			    dwc->suspended)
> 
> If dwc is already suspended why do we return -EBUSY?
> Should this be !dwc->suspended?
> 

Hi Roger

Thank you for reviewing.
If dwc->suspended is true (i.e suspend event due to U3/L2 is received), 
I am actually breaking from this switch statement and returning 0.

>> +				break;
>>   			return -EBUSY;
>> +		}
>>   		break;
>>   	case DWC3_GCTL_PRTCAP_HOST:
>>   	default:
>> @@ -2107,9 +2118,22 @@ static int dwc3_runtime_suspend(struct device *dev)
>>   	struct dwc3     *dwc = dev_get_drvdata(dev);
>>   	int		ret;
>>   
>> -	if (dwc3_runtime_checks(dwc))
>> +	ret = dwc3_runtime_checks(dwc);
>> +	if (ret)
>>   		return -EBUSY;
>>   
>> +	switch (dwc->current_dr_role) {
>> +	case DWC3_GCTL_PRTCAP_DEVICE:
>> +		/* bus suspend case */
>> +		if (!ret && dwc->connected)
> 
> No need to check !ret again as it will never happen because
> we are returning -EBUSY earlier if (ret);
> 
Thanks for this catch. I will remove !ret check in v5.

>> +			return 0;
>> +		break;
>> +	case DWC3_GCTL_PRTCAP_HOST:
>> +	default:
>> +		/* do nothing */
>> +		break;
>> +	}
>> +
> 
> While this takes care of runtime suspend case, what about system_suspend?
> Should this check be moved to dwc3_suspend_common() instead?
> 

Sure I can move these checks to dwc3_suspend_common to make it generic.
Will rename this patch to "Modify pm ops to handle bus suspend" since 
this is now not limited to only runtime suspend/resume. Will also rename 
dwc->runtime_suspend_on_usb_suspend to dwc->delegate_wakeup_interrupt 
based on earlier feedback.

I am still working on a clean way to enable/disable this feature (i.e 
set dwc->delegate_wakeup_interrupt flag) from the glue driver based on 
Thinh's feedback .
I will accommodate above feedback as well and upload v5.

Thanks
Elson
>>   	ret = dwc3_suspend_common(dwc, PMSG_AUTO_SUSPEND);
>>   	if (ret)
>>   		return ret;
>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>> index a69ac67d89fe..f2f788a6b4b5 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -1124,6 +1124,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.
>> + * @runtime_suspend_on_usb_suspend: true if dwc3 runtime suspend is allowed
>> + *			during bus suspend scenario.
>>    */
>>   struct dwc3 {
>>   	struct work_struct	drd_work;
>> @@ -1340,6 +1342,7 @@ struct dwc3 {
>>   	int			last_fifo_depth;
>>   	int			num_ep_resized;
>>   	struct dentry		*debug_root;
>> +	bool			runtime_suspend_on_usb_suspend;
>>   };
>>   
>>   #define INCRX_BURST_MODE 0
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 5fd067151fbf..978ce0e91164 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,
> 

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

[Roger Quadros]

On 24/10/2023 21:41, Elson Serrao wrote:
> 
> 
> On 10/24/2023 3:14 AM, Roger Quadros wrote:
>> Hi Elson,
>>
>> On 14/08/2023 21:50, Elson Roy Serrao wrote:
>>> The current implementation blocks the runtime pm operations when cable
>>> is connected. This would block dwc3 to enter a low power state 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   | 28 ++++++++++++++++++++++++++--
>>>   drivers/usb/dwc3/core.h   |  3 +++
>>>   drivers/usb/dwc3/gadget.c | 32 +++++++++++++++++++++++++-------
>>>   3 files changed, 54 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>> index 9c6bf054f15d..9bfd9bb18caf 100644
>>> --- a/drivers/usb/dwc3/core.c
>>> +++ b/drivers/usb/dwc3/core.c
>>> @@ -1518,6 +1518,9 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>>>       dwc->dis_split_quirk = device_property_read_bool(dev,
>>>                   "snps,dis-split-quirk");
>>>   +    dwc->runtime_suspend_on_usb_suspend = device_property_read_bool(dev,
>>> +                "snps,runtime-suspend-on-usb-suspend");
>>> +
>>>       dwc->lpm_nyet_threshold = lpm_nyet_threshold;
>>>       dwc->tx_de_emphasis = tx_de_emphasis;
>>>   @@ -2029,6 +2032,9 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
>>>         switch (dwc->current_dr_role) {
>>>       case DWC3_GCTL_PRTCAP_DEVICE:
>>> +        /* runtime resume on bus resume scenario */
>>> +        if (PMSG_IS_AUTO(msg) && dwc->connected)
>>> +            break;
>>>           ret = dwc3_core_init_for_resume(dwc);
>>>           if (ret)
>>>               return ret;
>>> @@ -2090,8 +2096,13 @@ static int dwc3_runtime_checks(struct dwc3 *dwc)
>>>   {
>>>       switch (dwc->current_dr_role) {
>>>       case DWC3_GCTL_PRTCAP_DEVICE:
>>> -        if (dwc->connected)
>>> +        if (dwc->connected) {
>>> +            /* bus suspend scenario */
>>> +            if (dwc->runtime_suspend_on_usb_suspend &&
>>> +                dwc->suspended)
>>
>> If dwc is already suspended why do we return -EBUSY?
>> Should this be !dwc->suspended?
>>
> 
> Hi Roger
> 
> Thank you for reviewing.
> If dwc->suspended is true (i.e suspend event due to U3/L2 is received), I am actually breaking from this switch statement and returning 0.

Of course. I missed the break :)

> 
>>> +                break;
>>>               return -EBUSY;
>>> +        }
>>>           break;
>>>       case DWC3_GCTL_PRTCAP_HOST:
>>>       default:
>>> @@ -2107,9 +2118,22 @@ static int dwc3_runtime_suspend(struct device *dev)
>>>       struct dwc3     *dwc = dev_get_drvdata(dev);
>>>       int        ret;
>>>   -    if (dwc3_runtime_checks(dwc))
>>> +    ret = dwc3_runtime_checks(dwc);
>>> +    if (ret)
>>>           return -EBUSY;
>>>   +    switch (dwc->current_dr_role) {
>>> +    case DWC3_GCTL_PRTCAP_DEVICE:
>>> +        /* bus suspend case */
>>> +        if (!ret && dwc->connected)
>>
>> No need to check !ret again as it will never happen because
>> we are returning -EBUSY earlier if (ret);
>>
> Thanks for this catch. I will remove !ret check in v5.
> 
>>> +            return 0;
>>> +        break;
>>> +    case DWC3_GCTL_PRTCAP_HOST:
>>> +    default:
>>> +        /* do nothing */
>>> +        break;
>>> +    }
>>> +
>>
>> While this takes care of runtime suspend case, what about system_suspend?
>> Should this check be moved to dwc3_suspend_common() instead?
>>
> 
> Sure I can move these checks to dwc3_suspend_common to make it generic.

Before you do that let's first decide how we want the gadget driver to behave
in system_suspend case.

Current behavior is to Disconnect from the Host.

Earlier I was thinking on the lines that we prevent system suspend if
we are not already in USB suspend. But I'm not sure if that is the right
thing to do anymore. Mainly because, system suspend is a result of user
request and it may not be nice to not to meet his/her request.
Maybe best to leave this policy handling to user space?
i.e. if user wants USB gadget operation to be alive, he will not issue
system suspend?


> Will rename this patch to "Modify pm ops to handle bus suspend" since this is now not limited to only runtime suspend/resume. Will also rename dwc->runtime_suspend_on_usb_suspend to dwc->delegate_wakeup_interrupt based on earlier feedback.
> 
> I am still working on a clean way to enable/disable this feature (i.e set dwc->delegate_wakeup_interrupt flag) from the glue driver based on Thinh's feedback .
> I will accommodate above feedback as well and upload v5.
> 
> Thanks
> Elson

-- 
cheers,
-roger

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

[Elson Serrao]

On 10/25/2023 1:02 AM, Roger Quadros wrote:
> 
> 
> On 24/10/2023 21:41, Elson Serrao wrote:
>>
>>
>> On 10/24/2023 3:14 AM, Roger Quadros wrote:
>>> Hi Elson,
>>>
>>> On 14/08/2023 21:50, Elson Roy Serrao wrote:
>>>> The current implementation blocks the runtime pm operations when cable
>>>> is connected. This would block dwc3 to enter a low power state 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   | 28 ++++++++++++++++++++++++++--
>>>>    drivers/usb/dwc3/core.h   |  3 +++
>>>>    drivers/usb/dwc3/gadget.c | 32 +++++++++++++++++++++++++-------
>>>>    3 files changed, 54 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>> index 9c6bf054f15d..9bfd9bb18caf 100644
>>>> --- a/drivers/usb/dwc3/core.c
>>>> +++ b/drivers/usb/dwc3/core.c
>>>> @@ -1518,6 +1518,9 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>>>>        dwc->dis_split_quirk = device_property_read_bool(dev,
>>>>                    "snps,dis-split-quirk");
>>>>    +    dwc->runtime_suspend_on_usb_suspend = device_property_read_bool(dev,
>>>> +                "snps,runtime-suspend-on-usb-suspend");
>>>> +
>>>>        dwc->lpm_nyet_threshold = lpm_nyet_threshold;
>>>>        dwc->tx_de_emphasis = tx_de_emphasis;
>>>>    @@ -2029,6 +2032,9 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
>>>>          switch (dwc->current_dr_role) {
>>>>        case DWC3_GCTL_PRTCAP_DEVICE:
>>>> +        /* runtime resume on bus resume scenario */
>>>> +        if (PMSG_IS_AUTO(msg) && dwc->connected)
>>>> +            break;
>>>>            ret = dwc3_core_init_for_resume(dwc);
>>>>            if (ret)
>>>>                return ret;
>>>> @@ -2090,8 +2096,13 @@ static int dwc3_runtime_checks(struct dwc3 *dwc)
>>>>    {
>>>>        switch (dwc->current_dr_role) {
>>>>        case DWC3_GCTL_PRTCAP_DEVICE:
>>>> -        if (dwc->connected)
>>>> +        if (dwc->connected) {
>>>> +            /* bus suspend scenario */
>>>> +            if (dwc->runtime_suspend_on_usb_suspend &&
>>>> +                dwc->suspended)
>>>
>>> If dwc is already suspended why do we return -EBUSY?
>>> Should this be !dwc->suspended?
>>>
>>
>> Hi Roger
>>
>> Thank you for reviewing.
>> If dwc->suspended is true (i.e suspend event due to U3/L2 is received), I am actually breaking from this switch statement and returning 0.
> 
> Of course. I missed the break :)
> 
>>
>>>> +                break;
>>>>                return -EBUSY;
>>>> +        }
>>>>            break;
>>>>        case DWC3_GCTL_PRTCAP_HOST:
>>>>        default:
>>>> @@ -2107,9 +2118,22 @@ static int dwc3_runtime_suspend(struct device *dev)
>>>>        struct dwc3     *dwc = dev_get_drvdata(dev);
>>>>        int        ret;
>>>>    -    if (dwc3_runtime_checks(dwc))
>>>> +    ret = dwc3_runtime_checks(dwc);
>>>> +    if (ret)
>>>>            return -EBUSY;
>>>>    +    switch (dwc->current_dr_role) {
>>>> +    case DWC3_GCTL_PRTCAP_DEVICE:
>>>> +        /* bus suspend case */
>>>> +        if (!ret && dwc->connected)
>>>
>>> No need to check !ret again as it will never happen because
>>> we are returning -EBUSY earlier if (ret);
>>>
>> Thanks for this catch. I will remove !ret check in v5.
>>
>>>> +            return 0;
>>>> +        break;
>>>> +    case DWC3_GCTL_PRTCAP_HOST:
>>>> +    default:
>>>> +        /* do nothing */
>>>> +        break;
>>>> +    }
>>>> +
>>>
>>> While this takes care of runtime suspend case, what about system_suspend?
>>> Should this check be moved to dwc3_suspend_common() instead?
>>>
>>
>> Sure I can move these checks to dwc3_suspend_common to make it generic.
> 
> Before you do that let's first decide how we want the gadget driver to behave
> in system_suspend case.
> 
> Current behavior is to Disconnect from the Host.
> 
> Earlier I was thinking on the lines that we prevent system suspend if
> we are not already in USB suspend. But I'm not sure if that is the right
> thing to do anymore. Mainly because, system suspend is a result of user
> request and it may not be nice to not to meet his/her request.

Agree. Irrespective of whether USB is suspended or not it is better to 
honor the system suspend request from user.

> Maybe best to leave this policy handling to user space?
> i.e. if user wants USB gadget operation to be alive, he will not issue
> system suspend?
> 

Sure. So below two cases

Case1: User doesn't care if gadget operation is alive and triggers 
system suspend irrespective of USB suspend. Like you mentioned, current 
behavior already takes care of this and initiates a DISCONNECT

Case2:  User wants gadget to stay alive and hence can trigger system 
suspend only when USB is suspended (there are already user space hooks 
that read cdev->suspended bit to tell whether USB is suspended or not 
for user to decide). Attempts to request system suspend when USB is not 
suspended, would result in a DISCONNECT.

For supporting Case2 from gadget driver point of view, we need to extend 
this series by having relevant checks in suspend_common()

Also, is it better to provide separate flags to control the gadget 
driver behavior for runtime suspend Vs system suspend when USB is 
suspended ? For example, what if we want to enable bus suspend handling 
for runtime suspend only and not for system suspend (Case1).

Thanks
Elson



> 
>> Will rename this patch to "Modify pm ops to handle bus suspend" since this is now not limited to only runtime suspend/resume. Will also rename dwc->runtime_suspend_on_usb_suspend to dwc->delegate_wakeup_interrupt based on earlier feedback.
>>
>> I am still working on a clean way to enable/disable this feature (i.e set dwc->delegate_wakeup_interrupt flag) from the glue driver based on Thinh's feedback .
>> I will accommodate above feedback as well and upload v5.
>>
>> Thanks
>> Elson
> 

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

[Roger Quadros]

On 26/10/2023 01:21, Elson Serrao wrote:
> 
> 
> On 10/25/2023 1:02 AM, Roger Quadros wrote:
>>
>>
>> On 24/10/2023 21:41, Elson Serrao wrote:
>>>
>>>
>>> On 10/24/2023 3:14 AM, Roger Quadros wrote:
>>>> Hi Elson,
>>>>
>>>> On 14/08/2023 21:50, Elson Roy Serrao wrote:
>>>>> The current implementation blocks the runtime pm operations when cable
>>>>> is connected. This would block dwc3 to enter a low power state 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   | 28 ++++++++++++++++++++++++++--
>>>>>    drivers/usb/dwc3/core.h   |  3 +++
>>>>>    drivers/usb/dwc3/gadget.c | 32 +++++++++++++++++++++++++-------
>>>>>    3 files changed, 54 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>>> index 9c6bf054f15d..9bfd9bb18caf 100644
>>>>> --- a/drivers/usb/dwc3/core.c
>>>>> +++ b/drivers/usb/dwc3/core.c
>>>>> @@ -1518,6 +1518,9 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>>>>>        dwc->dis_split_quirk = device_property_read_bool(dev,
>>>>>                    "snps,dis-split-quirk");
>>>>>    +    dwc->runtime_suspend_on_usb_suspend = device_property_read_bool(dev,
>>>>> +                "snps,runtime-suspend-on-usb-suspend");
>>>>> +
>>>>>        dwc->lpm_nyet_threshold = lpm_nyet_threshold;
>>>>>        dwc->tx_de_emphasis = tx_de_emphasis;
>>>>>    @@ -2029,6 +2032,9 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
>>>>>          switch (dwc->current_dr_role) {
>>>>>        case DWC3_GCTL_PRTCAP_DEVICE:
>>>>> +        /* runtime resume on bus resume scenario */
>>>>> +        if (PMSG_IS_AUTO(msg) && dwc->connected)
>>>>> +            break;
>>>>>            ret = dwc3_core_init_for_resume(dwc);
>>>>>            if (ret)
>>>>>                return ret;
>>>>> @@ -2090,8 +2096,13 @@ static int dwc3_runtime_checks(struct dwc3 *dwc)
>>>>>    {
>>>>>        switch (dwc->current_dr_role) {
>>>>>        case DWC3_GCTL_PRTCAP_DEVICE:
>>>>> -        if (dwc->connected)
>>>>> +        if (dwc->connected) {
>>>>> +            /* bus suspend scenario */
>>>>> +            if (dwc->runtime_suspend_on_usb_suspend &&
>>>>> +                dwc->suspended)
>>>>
>>>> If dwc is already suspended why do we return -EBUSY?
>>>> Should this be !dwc->suspended?
>>>>
>>>
>>> Hi Roger
>>>
>>> Thank you for reviewing.
>>> If dwc->suspended is true (i.e suspend event due to U3/L2 is received), I am actually breaking from this switch statement and returning 0.
>>
>> Of course. I missed the break :)
>>
>>>
>>>>> +                break;
>>>>>                return -EBUSY;
>>>>> +        }
>>>>>            break;
>>>>>        case DWC3_GCTL_PRTCAP_HOST:
>>>>>        default:
>>>>> @@ -2107,9 +2118,22 @@ static int dwc3_runtime_suspend(struct device *dev)
>>>>>        struct dwc3     *dwc = dev_get_drvdata(dev);
>>>>>        int        ret;
>>>>>    -    if (dwc3_runtime_checks(dwc))
>>>>> +    ret = dwc3_runtime_checks(dwc);
>>>>> +    if (ret)
>>>>>            return -EBUSY;
>>>>>    +    switch (dwc->current_dr_role) {
>>>>> +    case DWC3_GCTL_PRTCAP_DEVICE:
>>>>> +        /* bus suspend case */
>>>>> +        if (!ret && dwc->connected)
>>>>
>>>> No need to check !ret again as it will never happen because
>>>> we are returning -EBUSY earlier if (ret);
>>>>
>>> Thanks for this catch. I will remove !ret check in v5.
>>>
>>>>> +            return 0;
>>>>> +        break;
>>>>> +    case DWC3_GCTL_PRTCAP_HOST:
>>>>> +    default:
>>>>> +        /* do nothing */
>>>>> +        break;
>>>>> +    }
>>>>> +
>>>>
>>>> While this takes care of runtime suspend case, what about system_suspend?
>>>> Should this check be moved to dwc3_suspend_common() instead?
>>>>
>>>
>>> Sure I can move these checks to dwc3_suspend_common to make it generic.
>>
>> Before you do that let's first decide how we want the gadget driver to behave
>> in system_suspend case.
>>
>> Current behavior is to Disconnect from the Host.
>>
>> Earlier I was thinking on the lines that we prevent system suspend if
>> we are not already in USB suspend. But I'm not sure if that is the right
>> thing to do anymore. Mainly because, system suspend is a result of user
>> request and it may not be nice to not to meet his/her request.
> 
> Agree. Irrespective of whether USB is suspended or not it is better to honor the system suspend request from user.
> 
>> Maybe best to leave this policy handling to user space?
>> i.e. if user wants USB gadget operation to be alive, he will not issue
>> system suspend?
>>
> 
> Sure. So below two cases
> 
> Case1: User doesn't care if gadget operation is alive and triggers system suspend irrespective of USB suspend. Like you mentioned, current behavior already takes care of this and initiates a DISCONNECT
> 
> Case2:  User wants gadget to stay alive and hence can trigger system suspend only when USB is suspended (there are already user space hooks that read cdev->suspended bit to tell whether USB is suspended or not for user to decide). Attempts to request system suspend when USB is not suspended, would result in a DISCONNECT.
> 
> For supporting Case2 from gadget driver point of view, we need to extend this series by having relevant checks in suspend_common()
> 
> Also, is it better to provide separate flags to control the gadget driver behavior for runtime suspend Vs system suspend when USB is suspended ? For example, what if we want to enable bus suspend handling for runtime suspend only and not for system suspend (Case1).

But you mentioned that for Case1, USB gadget would disconnect from Host. So USB will be in disconnected state and USB controller can be fully de-activated? Except maybe wakeup handling to bring system out of suspend on a USB plug/unplug event?
Why do we need separate flags for?

-- 
cheers,
-roger

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

[Elson Serrao]

>>>>>
>>>>> While this takes care of runtime suspend case, what about system_suspend?
>>>>> Should this check be moved to dwc3_suspend_common() instead?
>>>>>
>>>>
>>>> Sure I can move these checks to dwc3_suspend_common to make it generic.
>>>
>>> Before you do that let's first decide how we want the gadget driver to behave
>>> in system_suspend case.
>>>
>>> Current behavior is to Disconnect from the Host.
>>>
>>> Earlier I was thinking on the lines that we prevent system suspend if
>>> we are not already in USB suspend. But I'm not sure if that is the right
>>> thing to do anymore. Mainly because, system suspend is a result of user
>>> request and it may not be nice to not to meet his/her request.
>>
>> Agree. Irrespective of whether USB is suspended or not it is better to honor the system suspend request from user.
>>
>>> Maybe best to leave this policy handling to user space?
>>> i.e. if user wants USB gadget operation to be alive, he will not issue
>>> system suspend?
>>>
>>
>> Sure. So below two cases
>>
>> Case1: User doesn't care if gadget operation is alive and triggers system suspend irrespective of USB suspend. Like you mentioned, current behavior already takes care of this and initiates a DISCONNECT
>>
>> Case2:  User wants gadget to stay alive and hence can trigger system suspend only when USB is suspended (there are already user space hooks that read cdev->suspended bit to tell whether USB is suspended or not for user to decide). Attempts to request system suspend when USB is not suspended, would result in a DISCONNECT.
>>
>> For supporting Case2 from gadget driver point of view, we need to extend this series by having relevant checks in suspend_common()
>>
>> Also, is it better to provide separate flags to control the gadget driver behavior for runtime suspend Vs system suspend when USB is suspended ? For example, what if we want to enable bus suspend handling for runtime suspend only and not for system suspend (Case1).
> 
> But you mentioned that for Case1, USB gadget would disconnect from Host. So USB will be in disconnected state and USB controller can be fully de-activated? Except maybe wakeup handling to bring system out of suspend on a USB plug/unplug event?
> Why do we need separate flags for?
> 

Sorry let me clarify. This is in reference to deciding how we want the 
dwc3 driver to behave in system_suspend case.

One option is to continue with the existing behavior where USB gadget 
would disconnect from Host irrespective of bus suspend state. We dont 
need any modification in this case and we can leave this series limited 
to runtime suspend only.

Second option is to stay connected IF we are in bus suspend state 
(U3/L2) otherwise DISCONNECT IF we are not in bus suspend state. The 
main motivation is to preserve the ongoing usb session
without going through a re-enumeration (ofcourse true only if we are in 
bus suspend state). This would need relevant checks in suspend_common().

Which option do you think is more suitable? IMO option2 is better. For 
example if we are in a scenario where there is a network session (over 
USB) open between Host and the device and usb bus is suspended due to 
data inactivity. Option2 would preserve the session whereas Option1 we 
would terminate this session when a system_suspend happens.

Thanks
Elson

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

[Roger Quadros]

On 27/10/2023 03:07, Elson Serrao wrote:
> 
> 
> 
>>>>>>
>>>>>> While this takes care of runtime suspend case, what about system_suspend?
>>>>>> Should this check be moved to dwc3_suspend_common() instead?
>>>>>>
>>>>>
>>>>> Sure I can move these checks to dwc3_suspend_common to make it generic.
>>>>
>>>> Before you do that let's first decide how we want the gadget driver to behave
>>>> in system_suspend case.
>>>>
>>>> Current behavior is to Disconnect from the Host.
>>>>
>>>> Earlier I was thinking on the lines that we prevent system suspend if
>>>> we are not already in USB suspend. But I'm not sure if that is the right
>>>> thing to do anymore. Mainly because, system suspend is a result of user
>>>> request and it may not be nice to not to meet his/her request.
>>>
>>> Agree. Irrespective of whether USB is suspended or not it is better to honor the system suspend request from user.
>>>
>>>> Maybe best to leave this policy handling to user space?
>>>> i.e. if user wants USB gadget operation to be alive, he will not issue
>>>> system suspend?
>>>>
>>>
>>> Sure. So below two cases
>>>
>>> Case1: User doesn't care if gadget operation is alive and triggers system suspend irrespective of USB suspend. Like you mentioned, current behavior already takes care of this and initiates a DISCONNECT
>>>
>>> Case2:  User wants gadget to stay alive and hence can trigger system suspend only when USB is suspended (there are already user space hooks that read cdev->suspended bit to tell whether USB is suspended or not for user to decide). Attempts to request system suspend when USB is not suspended, would result in a DISCONNECT.
>>>
>>> For supporting Case2 from gadget driver point of view, we need to extend this series by having relevant checks in suspend_common()
>>>
>>> Also, is it better to provide separate flags to control the gadget driver behavior for runtime suspend Vs system suspend when USB is suspended ? For example, what if we want to enable bus suspend handling for runtime suspend only and not for system suspend (Case1).
>>
>> But you mentioned that for Case1, USB gadget would disconnect from Host. So USB will be in disconnected state and USB controller can be fully de-activated? Except maybe wakeup handling to bring system out of suspend on a USB plug/unplug event?
>> Why do we need separate flags for?
>>
> 
> Sorry let me clarify. This is in reference to deciding how we want the dwc3 driver to behave in system_suspend case.
> 
> One option is to continue with the existing behavior where USB gadget would disconnect from Host irrespective of bus suspend state. We dont need any modification in this case and we can leave this series limited to runtime suspend only.
> 
> Second option is to stay connected IF we are in bus suspend state (U3/L2) otherwise DISCONNECT IF we are not in bus suspend state. The main motivation is to preserve the ongoing usb session
> without going through a re-enumeration (ofcourse true only if we are in bus suspend state). This would need relevant checks in suspend_common().

The catch here is, what to do if the USB device is not in bus suspend state but user wants to put the system in suspend state? Do we still disconnect?

You might also want to refer to the discussion in [1]

[1] - https://lore.kernel.org/all/Y+z9NK6AyhvTQMir@rowland.harvard.edu/

> 
> Which option do you think is more suitable? IMO option2 is better. For example if we are in a scenario where there is a network session (over USB) open between Host and the device and usb bus is suspended due to data inactivity. Option2 would preserve the session whereas Option1 we would terminate this session when a system_suspend happens.
> 
> Thanks
> Elson

-- 
cheers,
-roger

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

[Elson Serrao]

On 10/26/2023 11:37 PM, Roger Quadros wrote:
> 
> 
> On 27/10/2023 03:07, Elson Serrao wrote:
>>
>>
>>
>>>>>>>
>>>>>>> While this takes care of runtime suspend case, what about system_suspend?
>>>>>>> Should this check be moved to dwc3_suspend_common() instead?
>>>>>>>
>>>>>>
>>>>>> Sure I can move these checks to dwc3_suspend_common to make it generic.
>>>>>
>>>>> Before you do that let's first decide how we want the gadget driver to behave
>>>>> in system_suspend case.
>>>>>
>>>>> Current behavior is to Disconnect from the Host.
>>>>>
>>>>> Earlier I was thinking on the lines that we prevent system suspend if
>>>>> we are not already in USB suspend. But I'm not sure if that is the right
>>>>> thing to do anymore. Mainly because, system suspend is a result of user
>>>>> request and it may not be nice to not to meet his/her request.
>>>>
>>>> Agree. Irrespective of whether USB is suspended or not it is better to honor the system suspend request from user.
>>>>
>>>>> Maybe best to leave this policy handling to user space?
>>>>> i.e. if user wants USB gadget operation to be alive, he will not issue
>>>>> system suspend?
>>>>>
>>>>
>>>> Sure. So below two cases
>>>>
>>>> Case1: User doesn't care if gadget operation is alive and triggers system suspend irrespective of USB suspend. Like you mentioned, current behavior already takes care of this and initiates a DISCONNECT
>>>>
>>>> Case2:  User wants gadget to stay alive and hence can trigger system suspend only when USB is suspended (there are already user space hooks that read cdev->suspended bit to tell whether USB is suspended or not for user to decide). Attempts to request system suspend when USB is not suspended, would result in a DISCONNECT.
>>>>
>>>> For supporting Case2 from gadget driver point of view, we need to extend this series by having relevant checks in suspend_common()
>>>>
>>>> Also, is it better to provide separate flags to control the gadget driver behavior for runtime suspend Vs system suspend when USB is suspended ? For example, what if we want to enable bus suspend handling for runtime suspend only and not for system suspend (Case1).
>>>
>>> But you mentioned that for Case1, USB gadget would disconnect from Host. So USB will be in disconnected state and USB controller can be fully de-activated? Except maybe wakeup handling to bring system out of suspend on a USB plug/unplug event?
>>> Why do we need separate flags for?
>>>
>>
>> Sorry let me clarify. This is in reference to deciding how we want the dwc3 driver to behave in system_suspend case.
>>
>> One option is to continue with the existing behavior where USB gadget would disconnect from Host irrespective of bus suspend state. We dont need any modification in this case and we can leave this series limited to runtime suspend only.
>>
>> Second option is to stay connected IF we are in bus suspend state (U3/L2) otherwise DISCONNECT IF we are not in bus suspend state. The main motivation is to preserve the ongoing usb session
>> without going through a re-enumeration (ofcourse true only if we are in bus suspend state). This would need relevant checks in suspend_common().
> 
> The catch here is, what to do if the USB device is not in bus suspend state but user wants to put the system in suspend state? Do we still disconnect?
> 
> You might also want to refer to the discussion in [1]
> 
> [1] - https://lore.kernel.org/all/Y+z9NK6AyhvTQMir@rowland.harvard.edu/
> 

Thanks for the details. If we dont DISCONNECT when the USB link is NOT 
in bus suspend, the other alternatives we have are below ones

1.) Abort system_suspend: In addition to not honoring the users request 
to put the system in suspend, there is always a possibility of host 
driver never sending bus suspend interrupt. If that happens we would be 
denying system_suspend every time user requests it. So this is not a 
right approach.

2.) Keep the gadget connected and proceed with system_suspend: Now the 
system is suspended but from host point of view the USB link is active 
and would continue the communication normally. The signalling probably 
is not going to to be detected as a wakeup by the Rx hardware as there 
is no explicit resume signal involved here . The only exception is if we 
can somehow configure each and every event from the host as a wakeup 
signal(not sure if this is even possible)

So IMO it is best to DISCONNECT from the host when USB link is NOT in 
bus suspend state and user requests system_suspend.

Thanks
Elson