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

[Elson Serrao <quic_eserrao@xxxxxxxxxxx>]

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

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,