On Mon, Oct 14, 2019 at 02:13:55PM +0800, Daniel Drake wrote:
> On Asus laptops with AMD Ryzen7 3700U and AMD Ryzen5 3500U,
Can you include specific models here in case we revisit this or find a
generic solution that needs to be tested to make sure we don't regress
these platforms?
Maybe a bugzilla with complete "lspci -vvnn" and dmesg logs?
> the XHCI controller fails to resume from runtime suspend or s2idle,
> and USB becomes unusable from that point.
>
> xhci_hcd 0000:03:00.4: Refused to change power state, currently in D3
> xhci_hcd 0000:03:00.4: enabling device (0000 -> 0002)
> xhci_hcd 0000:03:00.4: WARN: xHC restore state timeout
> xhci_hcd 0000:03:00.4: PCI post-resume error -110!
> xhci_hcd 0000:03:00.4: HC died; cleaning up
>
> The D3-to-D0 transition is successful if the D3 delay is increased
> to 20ms. Add an appropriate quirk for the affected hardware.
IIUC, we're doing a D3cold-to-D0 transition in this path:
pci_pm_default_resume_early
pci_power_up
platform_pci_set_power_state(PCI_D0) # turn on via ACPI
pci_raw_set_power_state(PCI_D0)
pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr)
# pmcsr says device is in D3hot
pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, pmcsr)
# sets to D0
pci_dev_d3_sleep # <-- need more time here
I would sort of expect that ACPI would be putting the device in D0,
not leaving it in D3hot, but maybe that's just my ignorance.
In any case, I think you need more time after writing PCI_PM_CTRL to
transition from D3hot to D0. Right?
> Link: http://lkml.kernel.org/r/CAD8Lp47Vh69gQjROYG69=waJgL7hs1PwnLonL9+27S_TcRhixA@xxxxxxxxxxxxxx
> Signed-off-by: Daniel Drake <drake@xxxxxxxxxxxx>
> ---
> drivers/pci/quirks.c | 30 +++++++++++++++++++++++-------
> 1 file changed, 23 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 320255e5e8f8..4570439a6a6c 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -1871,19 +1871,35 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x2609, quirk_intel_pcie_pm);
> DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x260a, quirk_intel_pcie_pm);
> DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x260b, quirk_intel_pcie_pm);
>
> +static void quirk_d3_delay(struct pci_dev *dev, unsigned int delay)
> +{
> + if (dev->d3_delay >= delay)
> + return;
> +
> + dev->d3_delay = delay;
> + pci_info(dev, "extending delay after power-on from D3 to %d msec\n",
> + dev->d3_delay);
This delay is for a D3hot -> D0 transition, right? Can you make the
message say "D3hot" explicitly? And maybe include "d3hot" in the
function name as well?
> +}
This is a nice bit of refactoring; could you split it into a separate
patch that only does the refactor, followed by another that adds the
Ryzen quirk?
> static void quirk_radeon_pm(struct pci_dev *dev)
> {
> if (dev->subsystem_vendor == PCI_VENDOR_ID_APPLE &&
> - dev->subsystem_device == 0x00e2) {
> - if (dev->d3_delay < 20) {
> - dev->d3_delay = 20;
> - pci_info(dev, "extending delay after power-on from D3 to %d msec\n",
> - dev->d3_delay);
> - }
> - }
> + dev->subsystem_device == 0x00e2)
> + quirk_d3_delay(dev, 20);
> }
> DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x6741, quirk_radeon_pm);
>
> +/*
> + * Ryzen7 XHCI controllers fail upon resume from runtime suspend or s2idle
> + * unless an extended D3 delay is used.
> + */
> +static void quirk_ryzen_xhci_d3(struct pci_dev *dev)
> +{
> + quirk_d3_delay(dev, 20);
> +}
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AMD, 0x15e0, quirk_ryzen_xhci_d3);
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AMD, 0x15e1, quirk_ryzen_xhci_d3);
> +
> #ifdef CONFIG_X86_IO_APIC
> static int dmi_disable_ioapicreroute(const struct dmi_system_id *d)
> {
> --
> 2.20.1
>
