[+Kai]
Hi,
I don't understand the power management very well, so pardon my
ignorance but I have a question.
On Mon, May 10, 2021 at 3:30 AM Mika Westerberg
<mika.westerberg@xxxxxxxxxxxxxxx> wrote:
>
> ASMedia xHCI controller only supports PME from D3cold:
>
> 11:00.0 USB controller: ASMedia Technology Inc. ASM1042A USB 3.0 Host Controller (prog-if 30 [XHCI])
> ...
> Capabilities: [78] Power Management version 3
> Flags: PMEClk- DSI- D1- D2- AuxCurrent=55mA PME(D0-,D1-,D2-,D3hot-,D3cold+)
> Status: D0 NoSoftRst+ PME-Enable- DSel=0 DScale=0 PME-
>
> Now, if the controller is part of a Thunderbolt device for instance, it
> is connected to a PCIe switch downstream port. When the hierarchy then
> enters D3cold as a result of s2idle cycle pci_target_state() returns D0
> because the device does not support PME from the default target_state
> (D3hot). So what happens is that the whole hierarchy is left into D0
> breaking power management.at suspend time or resume time
Can you please provide a small call stack, when this issue is seen?
(I'm primarily trying to understand whether the issue is breaking
suspend, or the suspend is fine, but resume is broken?)
>
> For this reason choose target_state to be D3cold if there is a upstream
> bridge that is power manageable.
It seems to me that the goal of pci_target_state() is to find the
lowest power state that a device can be put into, from which device
can still generate PME (if needed). So I'm curious why it starts with
target_state = PCI_D3hot in the first place? Wouldn't starting with
PCI_D3cold will always be better (regardless of parent bridge
capabilities)?
And then I came across the commit 8feaec33b986 ("PCI / PM: Always
check PME wakeup capability for runtime wakeup support"), which
addresses the same device that this patch addresses, and 1 excerpt
from the commit log that stood out:
============================================================
In addition, change wakeup flag passed to pci_target_state() from false
to true, because we want to find the deepest state *different from D3cold*
that the device can still generate PME#. In this case, it's D0 for the
device in question.
============================================================
So, does returning D3Cold from this function break any other
assumption somewhere?
Thanks,
Rajat
> The reasoning here is that the upstream
> bridge will be also placed into D3 making the effective power state of
> the device in question to be D3cold.
>
> Reported-by: Utkarsh H Patel <utkarsh.h.patel@xxxxxxxxx>
> Reported-by: Koba Ko <koba.ko@xxxxxxxxxxxxx>
> Tested-by: Koba Ko <koba.ko@xxxxxxxxxxxxx>
> Signed-off-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
> ---
> drivers/pci/pci.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index b717680377a9..e3f3b9010762 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2578,8 +2578,19 @@ static pci_power_t pci_target_state(struct pci_dev *dev, bool wakeup)
> return target_state;
> }
>
> - if (!dev->pm_cap)
> + if (!dev->pm_cap) {
> target_state = PCI_D0;
> + } else {
> + struct pci_dev *bridge;
> +
> + /*
> + * If the upstream bridge can be put to D3 then it means
> + * that our target state is D3cold instead of D3hot.
> + */
> + bridge = pci_upstream_bridge(dev);
> + if (bridge && pci_bridge_d3_possible(bridge))
> + target_state = PCI_D3cold;
> + }
>
> /*
> * If the device is in D3cold even though it's not power-manageable by
> --
> 2.30.2
>
