| Message ID | 20240712185418.937087-2-superm1@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Put XHCI controllers into D3 at S4/S5 | expand |
On 12.7.2024 21.54, superm1@kernel.org wrote: > From: Mario Limonciello <mario.limonciello@amd.com> > > If a port on an XHCI controller hasn't been marked for wakeup at S4, then > leaving it at D0 will needlessly consume power than necessary. > > Explicitly check ports configured for wakeup and if none are found then > put the controller into D3hot before hibernate. > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > --- > drivers/usb/host/xhci-pci.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c > index 05881153883ec..4408d4caf66d2 100644 > --- a/drivers/usb/host/xhci-pci.c > +++ b/drivers/usb/host/xhci-pci.c > @@ -823,6 +823,7 @@ static int xhci_pci_poweroff_late(struct usb_hcd *hcd, bool do_wakeup) > struct usb_device *udev; > u32 portsc; > int i; > + bool wakeup = false; > > /* > * Systems with XHCI_RESET_TO_DEFAULT quirk have boot firmware that > @@ -860,6 +861,14 @@ static int xhci_pci_poweroff_late(struct usb_hcd *hcd, bool do_wakeup) > port->rhub->hcd->self.busnum, port->hcd_portnum + 1); > portsc = xhci_port_state_to_neutral(portsc); > writel(portsc | PORT_PE, port->addr); > + wakeup = true; > + } > + > + if (!wakeup) { > + struct pci_dev *pdev = to_pci_dev(hcd->self.controller); > + > + xhci_shutdown(hcd); > + pci_set_power_state(pdev, PCI_D3hot); Not sure we should force D3 here. I think usb core will set the PCI D state in the next .poweroff_noirq stage, for s4: .poweroff = hcd_pci_suspend, .poweroff_late = hcd_pci_poweroff_late, .poweroff_noirq = hcd_pci_suspend_noirq, hcd_pci_suspend_noirq() pci_prepare_to_sleep(); target_state = pci_target_state(dev, wakeup) pci_set_power_state(dev, target_state) Maybe the target_state isn't D3 for some reason? (missing ACPI entries?) Thanks Mathias
On 8/21/2024 04:25, Mathias Nyman wrote: > On 12.7.2024 21.54, superm1@kernel.org wrote: >> From: Mario Limonciello <mario.limonciello@amd.com> >> >> If a port on an XHCI controller hasn't been marked for wakeup at S4, then >> leaving it at D0 will needlessly consume power than necessary. >> >> Explicitly check ports configured for wakeup and if none are found then >> put the controller into D3hot before hibernate. >> >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> >> --- >> drivers/usb/host/xhci-pci.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c >> index 05881153883ec..4408d4caf66d2 100644 >> --- a/drivers/usb/host/xhci-pci.c >> +++ b/drivers/usb/host/xhci-pci.c >> @@ -823,6 +823,7 @@ static int xhci_pci_poweroff_late(struct usb_hcd >> *hcd, bool do_wakeup) >> struct usb_device *udev; >> u32 portsc; >> int i; >> + bool wakeup = false; >> /* >> * Systems with XHCI_RESET_TO_DEFAULT quirk have boot firmware that >> @@ -860,6 +861,14 @@ static int xhci_pci_poweroff_late(struct usb_hcd >> *hcd, bool do_wakeup) >> port->rhub->hcd->self.busnum, port->hcd_portnum + 1); >> portsc = xhci_port_state_to_neutral(portsc); >> writel(portsc | PORT_PE, port->addr); >> + wakeup = true; >> + } >> + >> + if (!wakeup) { >> + struct pci_dev *pdev = to_pci_dev(hcd->self.controller); >> + >> + xhci_shutdown(hcd); >> + pci_set_power_state(pdev, PCI_D3hot); > > Not sure we should force D3 here. > I think usb core will set the PCI D state in the next .poweroff_noirq > stage, > > for s4: > .poweroff = hcd_pci_suspend, > .poweroff_late = hcd_pci_poweroff_late, > .poweroff_noirq = hcd_pci_suspend_noirq, > > hcd_pci_suspend_noirq() > pci_prepare_to_sleep(); > target_state = pci_target_state(dev, wakeup) > pci_set_power_state(dev, target_state) > > Maybe the target_state isn't D3 for some reason? (missing ACPI entries?) > Thanks for looking. Even a lack of ACPI entries *should* still lead to D3hot if wakeup is turned off with that flow. I'll add some more debugging for the various callbacks and see what I can come up with to explain it. Can you please also review the other patch in the series? Thanks!
diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c index 05881153883ec..4408d4caf66d2 100644 --- a/drivers/usb/host/xhci-pci.c +++ b/drivers/usb/host/xhci-pci.c @@ -823,6 +823,7 @@ static int xhci_pci_poweroff_late(struct usb_hcd *hcd, bool do_wakeup) struct usb_device *udev; u32 portsc; int i; + bool wakeup = false; /* * Systems with XHCI_RESET_TO_DEFAULT quirk have boot firmware that @@ -860,6 +861,14 @@ static int xhci_pci_poweroff_late(struct usb_hcd *hcd, bool do_wakeup) port->rhub->hcd->self.busnum, port->hcd_portnum + 1); portsc = xhci_port_state_to_neutral(portsc); writel(portsc | PORT_PE, port->addr); + wakeup = true; + } + + if (!wakeup) { + struct pci_dev *pdev = to_pci_dev(hcd->self.controller); + + xhci_shutdown(hcd); + pci_set_power_state(pdev, PCI_D3hot); } return 0;