| Message ID | 1619586716-8687-2-git-send-email-sanm@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded |
| 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/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? > +int xhci_dwc3_suspend_quirk(struct usb_hcd *hcd) who calls this?
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.
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?
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 :-)
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 :)
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, +}; static int dwc3_host_get_irq(struct dwc3 *dwc) { @@ -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); + + 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); +} + +int xhci_dwc3_suspend_quirk(struct usb_hcd *hcd) +{ + struct dwc3 *dwc = dev_get_drvdata(hcd->self.controller->parent); + + dwc3_set_phy_mode(hcd); + + if (usb_wakeup_enabled_descendants(hcd->self.root_hub)) + dwc->phy_power_off = false; + else + dwc->phy_power_off = true; + + return 0; +} + void dwc3_host_exit(struct dwc3 *dwc) { platform_device_unregister(dwc->xhci);
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(+)