Re: [PATCH v2 8/9] usb: dwc3: qcom: fix wakeup implementation

[Johan Hovold <johan@kernel.org>]

On Sat, Aug 06, 2022 at 08:27:19PM +0530, Manivannan Sadhasivam wrote:
> On Thu, Aug 04, 2022 at 05:10:00PM +0200, Johan Hovold wrote:
> > It is the Qualcomm glue wakeup interrupts that may be able to wake the
> > system from suspend and this can now be described in the devicetree.
> > 
> > Move the wakeup-source property handling over from the core driver and
> > instead propagate the capability setting to the core device during
> > probe.

> The "wakeup-source" property is still defined in the DWC binding, so other
> platform glue drivers are free to assume that wakeup capability is handled by
> the DWC driver.

No, just because the binding says that the hardware supports something
doesn't mean it's implemented. And in this case it isn't.

There's no core support for wakeup and this is just used to determine
the PHY power state during suspend (see my reply to Matthias).

But this is also why I initially suggested reverting the binding change
until some platform actually turns out to need it.
 
> > This is needed as there is currently no way for the core driver to query
> > the wakeup setting of the glue device, but it is the core driver that
> > manages the PHY power state during suspend.
> > 
> > Also don't leave the PHYs enabled when system wakeup has been disabled
> > through sysfs.
> > 
> 
> Can you please elaborate on how this is handled in the patch?

Generally device_can_wakeup() should not be used to make policy
decisions (e.g. whether to power of the PHYs) as this should be
configurable through sysfs which is honoured by device_may_wakeup().

But I'll revisit this in a couple of weeks. We should probably just
revert the patch that made the PHY power state depend on
device_can_wakeup() as it apparently isn't needed for wakeup at all.

> > Fixes: 649f5c842ba3 ("usb: dwc3: core: Host wake up support from system suspend")
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > ---
> >  drivers/usb/dwc3/core.c      | 5 ++---
> >  drivers/usb/dwc3/dwc3-qcom.c | 6 +++++-
> >  2 files changed, 7 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > index 16d1f328775f..8c8e32651473 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -1822,7 +1822,6 @@ static int dwc3_probe(struct platform_device *pdev)
> >  
> >  	platform_set_drvdata(pdev, dwc);
> >  	dwc3_cache_hwparams(dwc);
> > -	device_init_wakeup(&pdev->dev, of_property_read_bool(dev->of_node, "wakeup-source"));
> >  
> >  	spin_lock_init(&dwc->lock);
> >  	mutex_init(&dwc->mutex);
> > @@ -1984,7 +1983,7 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
> >  		dwc3_core_exit(dwc);
> >  		break;
> >  	case DWC3_GCTL_PRTCAP_HOST:
> > -		if (!PMSG_IS_AUTO(msg) && !device_can_wakeup(dwc->dev)) {
> > +		if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(dwc->dev)) {
> >  			dwc3_core_exit(dwc);
> >  			break;
> >  		}
> > @@ -2045,7 +2044,7 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
> >  		spin_unlock_irqrestore(&dwc->lock, flags);
> >  		break;
> >  	case DWC3_GCTL_PRTCAP_HOST:
> > -		if (!PMSG_IS_AUTO(msg) && !device_can_wakeup(dwc->dev)) {
> > +		if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(dwc->dev)) {
> >  			ret = dwc3_core_init_for_resume(dwc);
> >  			if (ret)
> >  				return ret;
> > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> > index 6ae0b7fc4e2c..b05f67d206d2 100644
> > --- a/drivers/usb/dwc3/dwc3-qcom.c
> > +++ b/drivers/usb/dwc3/dwc3-qcom.c
> > @@ -786,6 +786,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
> >  	struct resource		*res, *parent_res = NULL;
> >  	int			ret, i;
> >  	bool			ignore_pipe_clk;
> > +	bool			wakeup_source;
> >  
> >  	qcom = devm_kzalloc(&pdev->dev, sizeof(*qcom), GFP_KERNEL);
> >  	if (!qcom)
> > @@ -901,7 +902,10 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
> >  	if (ret)
> >  		goto interconnect_exit;
> >  
> > -	device_init_wakeup(&pdev->dev, 1);
> > +	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);
> > +
> >  	qcom->is_suspended = false;
> >  	pm_runtime_set_active(dev);
> >  	pm_runtime_enable(dev);
> > -- 
> > 2.35.1

Johan