diff mbox series

[1/2] xhci: pci: If no ports have wakeup enabled then disable PCI device at S4

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

Commit Message

Mario Limonciello July 12, 2024, 6:54 p.m. UTC
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(+)

Comments

Mathias Nyman Aug. 21, 2024, 9:25 a.m. UTC | #1
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
Mario Limonciello Aug. 21, 2024, 8:59 p.m. UTC | #2
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 mbox series

Patch

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;