Re: [PATCH v7 1/5] usb: dwc3: host: Set PHY mode during suspend

From: Matthias Kaehlcke <mka@chromium.org>
To: Felipe Balbi <balbi@kernel.org>
Cc: Sandeep Maheswaram <sanm@codeaurora.org>,
	Andy Gross <agross@kernel.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Stephen Boyd <swboyd@chromium.org>,
	Doug Anderson <dianders@chromium.org>,
	linux-arm-msm@vger.kernel.org, linux-usb@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	Manu Gautam <mgautam@codeaurora.org>
Subject: Re: [PATCH v7 1/5] usb: dwc3: host: Set PHY mode during suspend
Date: Wed, 12 May 2021 12:12:26 -0700	[thread overview]
Message-ID: <YJwommGqKVeMdXth@google.com> (raw)
In-Reply-To: <87tunqka2e.fsf@kernel.org>

On Wed, Apr 28, 2021 at 12:55:21PM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> Sandeep Maheswaram <sanm@codeaurora.org> writes:
> > During suspend read the status of all port and make sure the PHYs
> > are in the correct mode based on current speed.
> > Phy interrupt masks are set based on this mode. Keep track of the mode
> > of the HS PHY to be able to configure wakeup properly.
> >
> > Also check during suspend if any wakeup capable devices are
> > connected to the controller (directly or through hubs), if there
> > are none set a flag to indicate that the PHY should be powered
> > down during suspend.
> >
> > Signed-off-by: Sandeep Maheswaram <sanm@codeaurora.org>
> > ---
> >  drivers/usb/dwc3/core.h |  3 +++
> >  drivers/usb/dwc3/host.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 62 insertions(+)
> >
> > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> > index b1e875c..cecd278 100644
> > --- a/drivers/usb/dwc3/core.h
> > +++ b/drivers/usb/dwc3/core.h
> > @@ -1123,6 +1123,9 @@ struct dwc3 {
> >  
> >  	bool			phys_ready;
> >  
> > +	unsigned int            hs_phy_mode;
> > +	bool			phy_power_off;
> > +
> >  	struct ulpi		*ulpi;
> >  	bool			ulpi_ready;
> >  
> > diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
> > index f29a264..527f04c 100644
> > --- a/drivers/usb/dwc3/host.c
> > +++ b/drivers/usb/dwc3/host.c
> > @@ -11,6 +11,14 @@
> >  #include <linux/platform_device.h>
> >  
> >  #include "core.h"
> > +#include "../host/xhci.h"
> > +#include "../host/xhci-plat.h"
> > +
> > +static int xhci_dwc3_suspend_quirk(struct usb_hcd *hcd);
> > +
> > +static const struct xhci_plat_priv xhci_plat_dwc3_xhci = {
> > +	.suspend_quirk = xhci_dwc3_suspend_quirk,
> > +};
> 
> we're passing data using device_properties, why do you want this here?
> 
> > @@ -115,6 +123,13 @@ int dwc3_host_init(struct dwc3 *dwc)
> >  		}
> >  	}
> >  
> > +	ret = platform_device_add_data(xhci, &xhci_plat_dwc3_xhci,
> > +			sizeof(struct xhci_plat_priv));
> > +	if (ret) {
> > +		dev_err(dwc->dev, "failed to add data to xHCI\n");
> > +		goto err;
> > +	}
> > +
> >  	ret = platform_device_add(xhci);
> >  	if (ret) {
> >  		dev_err(dwc->dev, "failed to register xHCI device\n");
> > @@ -127,6 +142,50 @@ int dwc3_host_init(struct dwc3 *dwc)
> >  	return ret;
> >  }
> >  
> > +static void dwc3_set_phy_mode(struct usb_hcd *hcd)
> > +{
> > +
> > +	int i, num_ports;
> > +	u32 reg;
> > +	unsigned int ss_phy_mode = 0;
> > +	struct dwc3 *dwc = dev_get_drvdata(hcd->self.controller->parent);
> > +	struct xhci_hcd	*xhci_hcd = hcd_to_xhci(hcd);
> > +
> > +	dwc->hs_phy_mode = 0;
> > +
> > +	reg = readl(&xhci_hcd->cap_regs->hcs_params1);
> > +	num_ports = HCS_MAX_PORTS(reg);
> 
> there's a big assumption here that xhci is still alive. Why isn't this
> quirk implemented in xhci-plat itself?

That should work for determining which types of devices are connected to
the PHYs, however IIUC the xhci-plat doesn't know about the PHY topology.
Are you suggesting to move that info into the xhci-plat driver so that it
can set the corresponding PHY modes?