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@xxxxxxxxxxx>
> ---
> 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
