| Message ID | 1624882097-23265-3-git-send-email-sanm@codeaurora.org (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | USB DWC3 host wake up support from system suspend | expand |
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/host/xhci-plat.c | 38 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 38 insertions(+) > > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c > index c1edcc9..ee87923 100644 > --- a/drivers/usb/host/xhci-plat.c > +++ b/drivers/usb/host/xhci-plat.c > @@ -24,6 +24,7 @@ > #include "xhci-plat.h" > #include "xhci-mvebu.h" > #include "xhci-rcar.h" > +#include "../dwc3/core.h" > > static struct hc_driver __read_mostly xhci_plat_hc_driver; > > @@ -430,6 +431,39 @@ static int xhci_plat_remove(struct platform_device *dev) > > return 0; > } > +static void xhci_dwc3_suspend_quirk(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; you're still bypassing the driver layering. First you had dwc access xhci, now you want xhci to access dwc. Either way is wrong. You need to rely on drivers core and device properties for this stuff. Don't access data you don't own.
Hi, On Mon, Jun 28, 2021 at 05:38:13PM +0530, Sandeep Maheswaram wrote: > 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. ... > --- a/drivers/usb/host/xhci-plat.c > +++ b/drivers/usb/host/xhci-plat.c > @@ -430,6 +431,39 @@ static int xhci_plat_remove(struct platform_device *dev) > > return 0; > } > +static void xhci_dwc3_suspend_quirk(struct usb_hcd *hcd) nit: you need a blank line above this (in between functions). > +{ > + 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); > + > + for (i = 0; i < num_ports; i++) { > + reg = readl(&xhci_hcd->op_regs->port_status_base + i * 0x04); > + if (reg & PORT_PE) { > + if (DEV_HIGHSPEED(reg) || DEV_FULLSPEED(reg)) > + dwc->hs_phy_mode |= PHY_MODE_USB_HOST_HS; > + else if (DEV_LOWSPEED(reg)) > + dwc->hs_phy_mode |= PHY_MODE_USB_HOST_LS; > + > + if (DEV_SUPERSPEED(reg)) > + ss_phy_mode |= PHY_MODE_USB_HOST_SS; > + } > + } > + phy_set_mode(dwc->usb2_generic_phy, dwc->hs_phy_mode); > + phy_set_mode(dwc->usb3_generic_phy, ss_phy_mode); > + > + if (usb_wakeup_enabled_descendants(hcd->self.root_hub)) > + dwc->phy_power_off = false; > + else > + dwc->phy_power_off = true; > +} This series breaks USB across S3 suspend/resume on Rockchip RK3399 Gru platforms. Those platforms do not support USB wake (they power off many of the relevant IP blocks in S3), and they *require* reconfiguring the PHY on resume, but usb_wakeup_enabled_descendants() is returning non-zero. If I revert patch 3, things work again. Perhaps that's a Rockchip bug (should such platforms be disabling all PM wakeup capabilities for their child hubs/devices?), but I'd much appreciate resolving that before merging such a regression. This also may be a sign that usb_wakeup_enabled_descendants() isn't really the precise condition you should be using, if other platforms aren't accurately reflecting feature support status in here. Brian P.S. In case it matters, I'm noticing this because earlier versions of your patches (which have the same problem) are merged in our downstream Chromium kernel tree.
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c index c1edcc9..ee87923 100644 --- a/drivers/usb/host/xhci-plat.c +++ b/drivers/usb/host/xhci-plat.c @@ -24,6 +24,7 @@ #include "xhci-plat.h" #include "xhci-mvebu.h" #include "xhci-rcar.h" +#include "../dwc3/core.h" static struct hc_driver __read_mostly xhci_plat_hc_driver; @@ -430,6 +431,39 @@ static int xhci_plat_remove(struct platform_device *dev) return 0; } +static void xhci_dwc3_suspend_quirk(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); + + for (i = 0; i < num_ports; i++) { + reg = readl(&xhci_hcd->op_regs->port_status_base + i * 0x04); + if (reg & PORT_PE) { + if (DEV_HIGHSPEED(reg) || DEV_FULLSPEED(reg)) + dwc->hs_phy_mode |= PHY_MODE_USB_HOST_HS; + else if (DEV_LOWSPEED(reg)) + dwc->hs_phy_mode |= PHY_MODE_USB_HOST_LS; + + if (DEV_SUPERSPEED(reg)) + ss_phy_mode |= PHY_MODE_USB_HOST_SS; + } + } + phy_set_mode(dwc->usb2_generic_phy, dwc->hs_phy_mode); + phy_set_mode(dwc->usb3_generic_phy, ss_phy_mode); + + if (usb_wakeup_enabled_descendants(hcd->self.root_hub)) + dwc->phy_power_off = false; + else + dwc->phy_power_off = true; +} static int __maybe_unused xhci_plat_suspend(struct device *dev) { @@ -440,6 +474,10 @@ static int __maybe_unused xhci_plat_suspend(struct device *dev) ret = xhci_priv_suspend_quirk(hcd); if (ret) return ret; + + if (of_device_is_compatible(dev->parent->of_node, "snps,dwc3")) + xhci_dwc3_suspend_quirk(hcd); + /* * xhci_suspend() needs `do_wakeup` to know whether host is allowed * to do wakeup during suspend.
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/host/xhci-plat.c | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+)