Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210204051850.64857-1-aaron.ma@canonical.com>
In-Reply-To: <20210204051850.64857-1-aaron.ma@canonical.com>
From:   Kai-Heng Feng <kai.heng.feng@canonical.com>
Date:   Fri, 5 Feb 2021 12:27:12 +0800
Message-ID: <CAAd53p4euFiw7pfDnD2H8oMVeeTqQ_c+wOFDLM2xPccn5MewiA@mail.gmail.com>
Subject: Re: [PATCH] xhci-pci: Set AMD Renoir USB controller to D3 when shutdown
To:     Aaron Ma <aaron.ma@canonical.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Mathias Nyman <mathias.nyman@linux.intel.com>,
        Alan Stern <stern@rowland.harvard.edu>, lee.jones@linaro.org,
        peter.chen@nxp.com, USB list <linux-usb@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Feb 4, 2021 at 1:20 PM Aaron Ma <aaron.ma@canonical.com> wrote:
>
> On AMD Renoir/Cezanne platforms, when set "Always on USB" to "On" in BIOS,
> USB controller will consume more power than 0.03w.
>
> Set it to D3cold when shutdown, S5 power consumption will be 0.03w lower.
> The USB can charge other devices as before.
> USB controller works fine after power on and reboot.

Can you please test the following patch, which should address the root cause:
https://lore.kernel.org/linux-acpi/20201201213019.1558738-1-furquan@google.com/

It also helps another AMD laptop on S5:
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1912935

We don't need to put bandage on drivers one by one once the patch with
alternative approach is in upstream.

Kai-Heng

>
> Signed-off-by: Aaron Ma <aaron.ma@canonical.com>
> ---
>  drivers/usb/host/xhci-pci.c | 8 ++++++++
>  drivers/usb/host/xhci.h     | 1 +
>  2 files changed, 9 insertions(+)
>
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> index 84da8406d5b4..a31be1ba927f 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -62,6 +62,7 @@
>  #define PCI_DEVICE_ID_AMD_PROMONTORYA_3                        0x43ba
>  #define PCI_DEVICE_ID_AMD_PROMONTORYA_2                        0x43bb
>  #define PCI_DEVICE_ID_AMD_PROMONTORYA_1                        0x43bc
> +#define PCI_DEVICE_ID_AMD_RENOIR_USB31                 0x1639
>  #define PCI_DEVICE_ID_ASMEDIA_1042_XHCI                        0x1042
>  #define PCI_DEVICE_ID_ASMEDIA_1042A_XHCI               0x1142
>  #define PCI_DEVICE_ID_ASMEDIA_1142_XHCI                        0x1242
> @@ -171,6 +172,10 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
>         if (pdev->vendor == PCI_VENDOR_ID_AMD)
>                 xhci->quirks |= XHCI_TRUST_TX_LENGTH;
>
> +       if (pdev->vendor == PCI_VENDOR_ID_AMD &&
> +               pdev->device == PCI_DEVICE_ID_AMD_RENOIR_USB31)
> +               xhci->quirks |= XHCI_SHUTDOWN_D3COLD;
> +
>         if ((pdev->vendor == PCI_VENDOR_ID_AMD) &&
>                 ((pdev->device == PCI_DEVICE_ID_AMD_PROMONTORYA_4) ||
>                 (pdev->device == PCI_DEVICE_ID_AMD_PROMONTORYA_3) ||
> @@ -594,6 +599,9 @@ static void xhci_pci_shutdown(struct usb_hcd *hcd)
>         /* Yet another workaround for spurious wakeups at shutdown with HSW */
>         if (xhci->quirks & XHCI_SPURIOUS_WAKEUP)
>                 pci_set_power_state(pdev, PCI_D3hot);
> +
> +       if (xhci->quirks & XHCI_SHUTDOWN_D3COLD)
> +               pci_set_power_state(pdev, PCI_D3cold);
>  }
>  #endif /* CONFIG_PM */
>
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index 25e57bc9c3cc..0684193da4bd 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -1883,6 +1883,7 @@ struct xhci_hcd {
>  #define XHCI_SKIP_PHY_INIT     BIT_ULL(37)
>  #define XHCI_DISABLE_SPARSE    BIT_ULL(38)
>  #define XHCI_SG_TRB_CACHE_SIZE_QUIRK   BIT_ULL(39)
> +#define XHCI_SHUTDOWN_D3COLD   BIT_ULL(40)
>
>         unsigned int            num_active_eps;
>         unsigned int            limit_active_eps;
> --
> 2.30.0
>