| Message ID | 1619586716-8687-1-git-send-email-sanm@codeaurora.org |
|---|---|
| Headers | show |
| Series | USB DWC3 host wake up support from system suspend | expand |
Hi, Sandeep Maheswaram <sanm@codeaurora.org> writes: > If wakeup capable devices are connected to the controller (directly > or through hubs) at suspend time keep the power domain on in order > to support wakeup from these devices. > > Signed-off-by: Sandeep Maheswaram <sanm@codeaurora.org> > --- > drivers/usb/dwc3/dwc3-qcom.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c > index 82125bc..1e220af 100644 > --- a/drivers/usb/dwc3/dwc3-qcom.c > +++ b/drivers/usb/dwc3/dwc3-qcom.c > @@ -17,9 +17,11 @@ > #include <linux/of_platform.h> > #include <linux/platform_device.h> > #include <linux/phy/phy.h> > +#include <linux/pm_domain.h> > #include <linux/usb/of.h> > #include <linux/reset.h> > #include <linux/iopoll.h> > +#include <linux/usb/hcd.h> > > #include "core.h" > > @@ -354,10 +356,19 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom) > { > u32 val; > int i, ret; > + struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3); > + struct usb_hcd *hcd; > + struct generic_pm_domain *genpd = pd_to_genpd(qcom->dev->pm_domain); > > if (qcom->is_suspended) > return 0; > > + if (dwc->xhci) { > + hcd = platform_get_drvdata(dwc->xhci); > + if (usb_wakeup_enabled_descendants(hcd->self.root_hub)) > + genpd->flags |= GENPD_FLAG_ACTIVE_WAKEUP; > + } wow, you really need to find a way to do these things generically instead of bypassing a bunch of layers and access stuff $this doesn't directly own. I'm gonna say 'no' to this, sorry. It looks like xhci should, directly, learn about much of this instead of hiding it 3-layers deep into the dwc3 glue layer for your specific SoC.
Hi, Sandeep Maheswaram <sanm@codeaurora.org> writes: >>> @@ -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? > Similar implemenation was done in > drivers/usb/cdns3/host.c<https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/usb/cdns3/host.c?h=next-20210503> then it seems we have two places to correct :-) >>> @@ -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? >> >>> +int xhci_dwc3_suspend_quirk(struct usb_hcd *hcd) >> who calls this? > This will be called from > xhci_plat_suspend->xhci_priv_suspend_quirk->xhci_dwc3_suspend_quirk So xhci.ko calls a function from dwc3.ko? That sounds odd, doesn't it? It really looks like we're just finding ways to bypass the driver layering, rather than working with it. -- balbi
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?
On Wed, Apr 28, 2021 at 01:04:43PM +0300, Felipe Balbi wrote: > > Hi, > > Sandeep Maheswaram <sanm@codeaurora.org> writes: > > If wakeup capable devices are connected to the controller (directly > > or through hubs) at suspend time keep the power domain on in order > > to support wakeup from these devices. > > > > Signed-off-by: Sandeep Maheswaram <sanm@codeaurora.org> > > --- > > drivers/usb/dwc3/dwc3-qcom.c | 17 +++++++++++++++++ > > 1 file changed, 17 insertions(+) > > > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c > > index 82125bc..1e220af 100644 > > --- a/drivers/usb/dwc3/dwc3-qcom.c > > +++ b/drivers/usb/dwc3/dwc3-qcom.c > > @@ -17,9 +17,11 @@ > > #include <linux/of_platform.h> > > #include <linux/platform_device.h> > > #include <linux/phy/phy.h> > > +#include <linux/pm_domain.h> > > #include <linux/usb/of.h> > > #include <linux/reset.h> > > #include <linux/iopoll.h> > > +#include <linux/usb/hcd.h> > > > > #include "core.h" > > > > @@ -354,10 +356,19 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom) > > { > > u32 val; > > int i, ret; > > + struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3); > > + struct usb_hcd *hcd; > > + struct generic_pm_domain *genpd = pd_to_genpd(qcom->dev->pm_domain); > > > > if (qcom->is_suspended) > > return 0; > > > > + if (dwc->xhci) { > > + hcd = platform_get_drvdata(dwc->xhci); > > + if (usb_wakeup_enabled_descendants(hcd->self.root_hub)) > > + genpd->flags |= GENPD_FLAG_ACTIVE_WAKEUP; > > + } > > wow, you really need to find a way to do these things generically > instead of bypassing a bunch of layers and access stuff $this doesn't > directly own. > > I'm gonna say 'no' to this, sorry. It looks like xhci should, directly, > learn about much of this instead of hiding it 3-layers deep into the > dwc3 glue layer for your specific SoC. Maybe this could be addressed with a pair of wakeup quirks, one for suspend, another for resume. An optional genpd field could be added to struct xhci_plat_priv. The wakeup quirks would set/clear GENPD_FLAG_ACTIVE_WAKEUP of the genpd. Does the above sound more palatable?
Hi, Matthias Kaehlcke <mka@chromium.org> writes: >> > @@ -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? Yes, if xHCI needs to know about PHYs in order to properly configure the PHYs, so be it :-) -- balbi
Hi, Matthias Kaehlcke <mka@chromium.org> writes: >> Sandeep Maheswaram <sanm@codeaurora.org> writes: >> > If wakeup capable devices are connected to the controller (directly >> > or through hubs) at suspend time keep the power domain on in order >> > to support wakeup from these devices. >> > >> > Signed-off-by: Sandeep Maheswaram <sanm@codeaurora.org> >> > --- >> > drivers/usb/dwc3/dwc3-qcom.c | 17 +++++++++++++++++ >> > 1 file changed, 17 insertions(+) >> > >> > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c >> > index 82125bc..1e220af 100644 >> > --- a/drivers/usb/dwc3/dwc3-qcom.c >> > +++ b/drivers/usb/dwc3/dwc3-qcom.c >> > @@ -17,9 +17,11 @@ >> > #include <linux/of_platform.h> >> > #include <linux/platform_device.h> >> > #include <linux/phy/phy.h> >> > +#include <linux/pm_domain.h> >> > #include <linux/usb/of.h> >> > #include <linux/reset.h> >> > #include <linux/iopoll.h> >> > +#include <linux/usb/hcd.h> >> > >> > #include "core.h" >> > >> > @@ -354,10 +356,19 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom) >> > { >> > u32 val; >> > int i, ret; >> > + struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3); >> > + struct usb_hcd *hcd; >> > + struct generic_pm_domain *genpd = pd_to_genpd(qcom->dev->pm_domain); >> > >> > if (qcom->is_suspended) >> > return 0; >> > >> > + if (dwc->xhci) { >> > + hcd = platform_get_drvdata(dwc->xhci); >> > + if (usb_wakeup_enabled_descendants(hcd->self.root_hub)) >> > + genpd->flags |= GENPD_FLAG_ACTIVE_WAKEUP; >> > + } >> >> wow, you really need to find a way to do these things generically >> instead of bypassing a bunch of layers and access stuff $this doesn't >> directly own. >> >> I'm gonna say 'no' to this, sorry. It looks like xhci should, directly, >> learn about much of this instead of hiding it 3-layers deep into the >> dwc3 glue layer for your specific SoC. > > Maybe this could be addressed with a pair of wakeup quirks, one for suspend, > another for resume. An optional genpd field could be added to struct > xhci_plat_priv. The wakeup quirks would set/clear GENPD_FLAG_ACTIVE_WAKEUP > of the genpd. > > Does the above sound more palatable? I don't get why you need quirk flags. All you're doing here is: if (usb_wakeup_enabled_descendants(hcd->self.root_hub)) genpd->flags |= GENPD_FLAG_ACTIVE_WAKEUP; If you move this test to xhci_suspend(), you shouldn't need all the magic, right? -- balbi
On Thu, May 13, 2021 at 04:49:19PM +0300, Felipe Balbi wrote: > > Hi, > > Matthias Kaehlcke <mka@chromium.org> writes: > >> Sandeep Maheswaram <sanm@codeaurora.org> writes: > >> > If wakeup capable devices are connected to the controller (directly > >> > or through hubs) at suspend time keep the power domain on in order > >> > to support wakeup from these devices. > >> > > >> > Signed-off-by: Sandeep Maheswaram <sanm@codeaurora.org> > >> > --- > >> > drivers/usb/dwc3/dwc3-qcom.c | 17 +++++++++++++++++ > >> > 1 file changed, 17 insertions(+) > >> > > >> > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c > >> > index 82125bc..1e220af 100644 > >> > --- a/drivers/usb/dwc3/dwc3-qcom.c > >> > +++ b/drivers/usb/dwc3/dwc3-qcom.c > >> > @@ -17,9 +17,11 @@ > >> > #include <linux/of_platform.h> > >> > #include <linux/platform_device.h> > >> > #include <linux/phy/phy.h> > >> > +#include <linux/pm_domain.h> > >> > #include <linux/usb/of.h> > >> > #include <linux/reset.h> > >> > #include <linux/iopoll.h> > >> > +#include <linux/usb/hcd.h> > >> > > >> > #include "core.h" > >> > > >> > @@ -354,10 +356,19 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom) > >> > { > >> > u32 val; > >> > int i, ret; > >> > + struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3); > >> > + struct usb_hcd *hcd; > >> > + struct generic_pm_domain *genpd = pd_to_genpd(qcom->dev->pm_domain); > >> > > >> > if (qcom->is_suspended) > >> > return 0; > >> > > >> > + if (dwc->xhci) { > >> > + hcd = platform_get_drvdata(dwc->xhci); > >> > + if (usb_wakeup_enabled_descendants(hcd->self.root_hub)) > >> > + genpd->flags |= GENPD_FLAG_ACTIVE_WAKEUP; > >> > + } > >> > >> wow, you really need to find a way to do these things generically > >> instead of bypassing a bunch of layers and access stuff $this doesn't > >> directly own. > >> > >> I'm gonna say 'no' to this, sorry. It looks like xhci should, directly, > >> learn about much of this instead of hiding it 3-layers deep into the > >> dwc3 glue layer for your specific SoC. > > > > Maybe this could be addressed with a pair of wakeup quirks, one for suspend, > > another for resume. An optional genpd field could be added to struct > > xhci_plat_priv. The wakeup quirks would set/clear GENPD_FLAG_ACTIVE_WAKEUP > > of the genpd. > > > > Does the above sound more palatable? > > I don't get why you need quirk flags. All you're doing here is: > > if (usb_wakeup_enabled_descendants(hcd->self.root_hub)) > genpd->flags |= GENPD_FLAG_ACTIVE_WAKEUP; > > If you move this test to xhci_suspend(), you shouldn't need all the > magic, right? Right, the quirks shouldn't be necessary if setting/clearing the genpd flag is the right thing to do for any controller that sets the genpd field in _priv, which probably is the case.
On Thu, May 13, 2021 at 04:46:41PM +0300, Felipe Balbi wrote: > > Hi, > > Matthias Kaehlcke <mka@chromium.org> writes: > >> > @@ -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? > > Yes, if xHCI needs to know about PHYs in order to properly configure the > PHYs, so be it :-) Thanks for the confirmation, looks like we have a path forward here then :)