Message-ID: <Ymw6Og/qhg3D0mx+@google.com>
Date:   Fri, 29 Apr 2022 12:19:22 -0700
From:   Matthias Kaehlcke <mka@...omium.org>
To:     Pavan Kondeti <quic_pkondeti@...cinc.com>
Cc:     "Rafael J. Wysocki" <rafael@...nel.org>,
        Sandeep Maheswaram <quic_c_sanm@...cinc.com>,
        Rob Herring <robh+dt@...nel.org>,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Felipe Balbi <balbi@...nel.org>,
        Stephen Boyd <swboyd@...omium.org>,
        Doug Anderson <dianders@...omium.org>,
        Mathias Nyman <mathias.nyman@...el.com>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Len Brown <len.brown@...el.com>, Pavel Machek <pavel@....cz>,
        Linux PM <linux-pm@...r.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        "open list:ULTRA-WIDEBAND (UWB) SUBSYSTEM:" 
        <linux-usb@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        quic_ppratap@...cinc.com, quic_kriskura@...cinc.com,
        quic_vpulyala@...cinc.com
Subject: Re: [PATCH v14 2/7] PM / wakeup: Add device_children_wakeup_capable()

Hi Pavan,

On Fri, Apr 29, 2022 at 06:29:56PM +0530, Pavan Kondeti wrote:
> Hi Matthias,
> 
> On Mon, Apr 25, 2022 at 06:33:03PM +0530, Pavan Kondeti wrote:
> > Hi Matthias,
> > 
> > On Fri, Apr 22, 2022 at 11:44:36AM -0700, Matthias Kaehlcke wrote:
> > > On Fri, Apr 22, 2022 at 01:57:17PM +0200, Rafael J. Wysocki wrote:
> > > > On Tue, Apr 19, 2022 at 9:11 PM Sandeep Maheswaram
> > > > <quic_c_sanm@...cinc.com> wrote:
> > > > >
> > > > > From: Matthias Kaehlcke <mka@...omium.org>
> > > > >
> > > > > Add device_children_wakeup_capable() which checks whether the device itself
> > > > > or one if its descendants is wakeup capable.
> > > > 
> > > > device_wakeup_path() exists for a very similar purpose.
> > > > 
> > > > Is it not usable for whatever you need the new function introduced here?
> > > 
> > > I wasn't aware of it's function, there are no doc comments and the
> > > name isn't really self explanatory.
> > > 
> > > In a quick test device_wakeup_path() returned inconsistent values for the
> > > root hub, sometimes true, others false when a wakeup capable USB device was
> > > connected.
> > 
> > We will also test the same to double confirm the behavior of
> > device_wakeup_path(). I am assuming that you checked device_wakeup_path()
> > only during system suspend path.
> > 
> > Here is what I understood by looking at __device_suspend(). Please share
> > your thoughts on this.
> > 
> > power.wakeup_path is set to true for the parent *after* a wakeup capable
> > device is suspended. This means when the root hub(s) is suspended, it is
> > propagated to xhci-plat and when xhci-plat is suspended, it is propagated
> > to dwc3. bottom up propgation during system suspend.
> > 
> > I believe we can directly check something like this in the dwc3 driver
> > instead of having another wrapper like device_children_wakeup_capable().
> > 
> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > index 1170b80..a783257 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -1878,8 +1878,14 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
> >  		break;
> >  	case DWC3_GCTL_PRTCAP_HOST:
> >  		if (!PMSG_IS_AUTO(msg)) {
> > +			/*
> > +			 * Don't kill the host when dwc3 is wakeup capable and
> > +			 * its children needs wakeup.
> > +			 */
> > +			if (device_may_wakeup(dwc->dev) && device_wakeup_path(dwc->dev))
> > +				handle_it();
> > +		} else {
> >  			dwc3_core_exit(dwc);
> > -			break;
> >  		}
> >  
> >  		/* Let controller to suspend HSPHY before PHY driver suspends */
> > 
> 
> device_wakeup_path(dwc->dev) is returning true all the time irrespective of
> the wakeup capability (and enabled status) of the connected USB devices. That
> is because xhci-plat device is configured to wakeup all the time. Since the
> child is wakeup capable, its parent i.e dwc3 has device_wakeup_path() set.
> device_children_wakeup_capable() will also suffer the problem. However,
> 
> device_children_wakeup_capable(&hcd->self.root_hub->dev) is what Sandeep's
> patch is using. That is not correct. we have two root hubs (HS and SS) associated
> with a USB3 controller and calling it on one root hub is incorrect. 
> device_children_wakeup_capable() must be called on xhci-plat so that it covers
> both HS and SS root hubs

Thanks for pointing that out!

> I am thinking of dynamically enabling/disabling xhci-plat wakeup capability so
> that the wakeup path is correctly propagated to dwc3. something like below.
> Does it make sense to you?
> 
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index 649ffd8..be0c55b 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -412,6 +412,9 @@ static int __maybe_unused xhci_plat_suspend(struct device *dev)
>  	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
>  	int ret;
>  
> +	if (!device_wakeup_path(dev))
> +		device_wakeup_disable(dev);
> +
>  	if (pm_runtime_suspended(dev))
>  		pm_runtime_resume(dev);
>  
> @@ -443,6 +446,8 @@ static int __maybe_unused xhci_plat_resume(struct device *dev)
>  	pm_runtime_set_active(dev);
>  	pm_runtime_enable(dev);
>  
> +	device_wakeup_enable(dev);

I think this also needs to be done conditionally, otherwise it would
create a new wake source on every resume when wakeup is already
enabled.

Other than that this seems to do the trick and keeps the USB layer out of
the dwc3 code.

Powered by blists - more mailing lists