| Message ID | 20240226152831.2147932-1-Basavaraj.Natikar@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] xhci: Allow RPM on the USB controller (1022:43f7) by default | expand |
On 2/26/2024 09:28, Basavaraj Natikar wrote: > The AMD USB host controller (1022:43f7) does not enter PCI D3 by default > when nothing is connected. This is due to the policy introduced by > 'commit a611bf473d1f ("xhci-pci: Set runtime PM as default policy on all > xHC 1.2 or later devices")', which only covers 1.2 or later devices. > > Therefore, by default, allow RPM on the AMD USB controller [1022:43f7]. > > Fixes: 4baf12181509 ("xhci: Loosen RPM as default policy to cover for AMD xHC 1.1") > Link: https://lore.kernel.org/all/12335218.O9o76ZdvQC@natalenko.name/ > Cc: Mario Limonciello <mario.limonciello@amd.com> > Cc: stable@vger.kernel.org > Tested-by: Oleksandr Natalenko <oleksandr@natalenko.name> Does Oleksandr's testing actually apply here? This is a totally different patch and system isn't it? > Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com> > --- > Changes in v2: > - Added Cc: stable@vger.kernel.org > > drivers/usb/host/xhci-pci.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c > index b534ca9752be..1eb7a41a75d7 100644 > --- a/drivers/usb/host/xhci-pci.c > +++ b/drivers/usb/host/xhci-pci.c > @@ -473,6 +473,8 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci) > /* xHC spec requires PCI devices to support D3hot and D3cold */ > if (xhci->hci_version >= 0x120) > xhci->quirks |= XHCI_DEFAULT_PM_RUNTIME_ALLOW; > + else if (pdev->vendor == PCI_VENDOR_ID_AMD && pdev->device == 0x43f7) > + xhci->quirks |= XHCI_DEFAULT_PM_RUNTIME_ALLOW; > > if (xhci->quirks & XHCI_RESET_ON_RESUME) > xhci_dbg_trace(xhci, trace_xhci_dbg_quirks,
On 2/26/2024 9:02 PM, Mario Limonciello wrote: > On 2/26/2024 09:28, Basavaraj Natikar wrote: >> The AMD USB host controller (1022:43f7) does not enter PCI D3 by default >> when nothing is connected. This is due to the policy introduced by >> 'commit a611bf473d1f ("xhci-pci: Set runtime PM as default policy on all >> xHC 1.2 or later devices")', which only covers 1.2 or later devices. >> >> Therefore, by default, allow RPM on the AMD USB controller [1022:43f7]. >> >> Fixes: 4baf12181509 ("xhci: Loosen RPM as default policy to cover for >> AMD xHC 1.1") >> Link: https://lore.kernel.org/all/12335218.O9o76ZdvQC@natalenko.name/ >> Cc: Mario Limonciello <mario.limonciello@amd.com> >> Cc: stable@vger.kernel.org >> Tested-by: Oleksandr Natalenko <oleksandr@natalenko.name> > > Does Oleksandr's testing actually apply here? This is a totally > different patch and system isn't it? This patch is added in https://lore.kernel.org/all/12335218.O9o76ZdvQC@natalenko.name/ And he mentioned in link https://lore.kernel.org/all/12335218.O9o76ZdvQC@natalenko.name/ to add Tested-by Thanks, -- Basavaraj > >> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com> >> --- >> Changes in v2: >> - Added Cc: stable@vger.kernel.org >> >> drivers/usb/host/xhci-pci.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c >> index b534ca9752be..1eb7a41a75d7 100644 >> --- a/drivers/usb/host/xhci-pci.c >> +++ b/drivers/usb/host/xhci-pci.c >> @@ -473,6 +473,8 @@ static void xhci_pci_quirks(struct device *dev, >> struct xhci_hcd *xhci) >> /* xHC spec requires PCI devices to support D3hot and D3cold */ >> if (xhci->hci_version >= 0x120) >> xhci->quirks |= XHCI_DEFAULT_PM_RUNTIME_ALLOW; >> + else if (pdev->vendor == PCI_VENDOR_ID_AMD && pdev->device == >> 0x43f7) >> + xhci->quirks |= XHCI_DEFAULT_PM_RUNTIME_ALLOW; >> if (xhci->quirks & XHCI_RESET_ON_RESUME) >> xhci_dbg_trace(xhci, trace_xhci_dbg_quirks, >
On 2/26/2024 09:37, Basavaraj Natikar wrote: > > On 2/26/2024 9:02 PM, Mario Limonciello wrote: >> On 2/26/2024 09:28, Basavaraj Natikar wrote: >>> The AMD USB host controller (1022:43f7) does not enter PCI D3 by default >>> when nothing is connected. This is due to the policy introduced by >>> 'commit a611bf473d1f ("xhci-pci: Set runtime PM as default policy on all >>> xHC 1.2 or later devices")', which only covers 1.2 or later devices. >>> >>> Therefore, by default, allow RPM on the AMD USB controller [1022:43f7]. >>> >>> Fixes: 4baf12181509 ("xhci: Loosen RPM as default policy to cover for >>> AMD xHC 1.1") >>> Link: https://lore.kernel.org/all/12335218.O9o76ZdvQC@natalenko.name/ >>> Cc: Mario Limonciello <mario.limonciello@amd.com> >>> Cc: stable@vger.kernel.org >>> Tested-by: Oleksandr Natalenko <oleksandr@natalenko.name> >> >> Does Oleksandr's testing actually apply here? This is a totally >> different patch and system isn't it? > > This patch is added in https://lore.kernel.org/all/12335218.O9o76ZdvQC@natalenko.name/ > > And he mentioned in link https://lore.kernel.org/all/12335218.O9o76ZdvQC@natalenko.name/ > to add Tested-by > Ah got it, thanks. > Thanks, > -- > Basavaraj > >> >>> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com> >>> --- >>> Changes in v2: >>> - Added Cc: stable@vger.kernel.org >>> >>> drivers/usb/host/xhci-pci.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c >>> index b534ca9752be..1eb7a41a75d7 100644 >>> --- a/drivers/usb/host/xhci-pci.c >>> +++ b/drivers/usb/host/xhci-pci.c >>> @@ -473,6 +473,8 @@ static void xhci_pci_quirks(struct device *dev, >>> struct xhci_hcd *xhci) >>> /* xHC spec requires PCI devices to support D3hot and D3cold */ >>> if (xhci->hci_version >= 0x120) >>> xhci->quirks |= XHCI_DEFAULT_PM_RUNTIME_ALLOW; >>> + else if (pdev->vendor == PCI_VENDOR_ID_AMD && pdev->device == >>> 0x43f7) >>> + xhci->quirks |= XHCI_DEFAULT_PM_RUNTIME_ALLOW; >>> if (xhci->quirks & XHCI_RESET_ON_RESUME) >>> xhci_dbg_trace(xhci, trace_xhci_dbg_quirks, >> >
On 26.2.2024 17.28, Basavaraj Natikar wrote: > The AMD USB host controller (1022:43f7) does not enter PCI D3 by default > when nothing is connected. This is due to the policy introduced by > 'commit a611bf473d1f ("xhci-pci: Set runtime PM as default policy on all > xHC 1.2 or later devices")', which only covers 1.2 or later devices. This makes it seem like commit a611bf473d1 somehow restricted default runtime PM when in fact it enabled it for all xHCI 1.2 hosts. Before that only a few selected ones had runtime PM enabled by default. How about something like: Enable runtime PM by default for older AMD 1022:43f7 xHCI 1.1 host as it is proven to work. Driver enables runtime PM by default for newer xHCI 1.2 host. > > Therefore, by default, allow RPM on the AMD USB controller [1022:43f7]. > > Fixes: 4baf12181509 ("xhci: Loosen RPM as default policy to cover for AMD xHC 1.1") This was already reverted as it caused regression on some systems. 24be0b3c4059 Revert "xhci: Loosen RPM as default policy to cover for AMD xHC 1.1" > Link: https://lore.kernel.org/all/12335218.O9o76ZdvQC@natalenko.name/ > Cc: Mario Limonciello <mario.limonciello@amd.com> > Cc: stable@vger.kernel.org I'd skip Fixes and stable tags and add this as a feature to usb-next. > Tested-by: Oleksandr Natalenko <oleksandr@natalenko.name> > Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com> > --- > Changes in v2: > - Added Cc: stable@vger.kernel.org > > drivers/usb/host/xhci-pci.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c > index b534ca9752be..1eb7a41a75d7 100644 > --- a/drivers/usb/host/xhci-pci.c > +++ b/drivers/usb/host/xhci-pci.c > @@ -473,6 +473,8 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci) > /* xHC spec requires PCI devices to support D3hot and D3cold */ > if (xhci->hci_version >= 0x120) > xhci->quirks |= XHCI_DEFAULT_PM_RUNTIME_ALLOW; > + else if (pdev->vendor == PCI_VENDOR_ID_AMD && pdev->device == 0x43f7) > + xhci->quirks |= XHCI_DEFAULT_PM_RUNTIME_ALLOW; This would fit better earlier in the code among the rest of the AMD quirks. See how this flag is set for some other hosts. Thanks Mathias
On 2/29/2024 3:16 PM, Mathias Nyman wrote: > On 26.2.2024 17.28, Basavaraj Natikar wrote: >> The AMD USB host controller (1022:43f7) does not enter PCI D3 by default >> when nothing is connected. This is due to the policy introduced by >> 'commit a611bf473d1f ("xhci-pci: Set runtime PM as default policy on all >> xHC 1.2 or later devices")', which only covers 1.2 or later devices. > > This makes it seem like commit a611bf473d1 somehow restricted default > runtime > PM when in fact it enabled it for all xHCI 1.2 hosts. > > Before that only a few selected ones had runtime PM enabled by default. > > How about something like: > > Enable runtime PM by default for older AMD 1022:43f7 xHCI 1.1 host as > it is > proven to work. > Driver enables runtime PM by default for newer xHCI 1.2 host. Thank you for the rewording. I will change accordingly. > >> >> Therefore, by default, allow RPM on the AMD USB controller [1022:43f7]. >> >> Fixes: 4baf12181509 ("xhci: Loosen RPM as default policy to cover for >> AMD xHC 1.1") > > This was already reverted as it caused regression on some systems. > 24be0b3c4059 Revert "xhci: Loosen RPM as default policy to cover for > AMD xHC 1.1" > >> Link: https://lore.kernel.org/all/12335218.O9o76ZdvQC@natalenko.name/ >> Cc: Mario Limonciello <mario.limonciello@amd.com> >> Cc: stable@vger.kernel.org > > I'd skip Fixes and stable tags and add this as a feature to usb-next. Sure, I will remove the above Fixes and Cc tag in the v3 patch. > >> Tested-by: Oleksandr Natalenko <oleksandr@natalenko.name> >> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com> >> --- >> Changes in v2: >> - Added Cc: stable@vger.kernel.org >> >> drivers/usb/host/xhci-pci.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c >> index b534ca9752be..1eb7a41a75d7 100644 >> --- a/drivers/usb/host/xhci-pci.c >> +++ b/drivers/usb/host/xhci-pci.c >> @@ -473,6 +473,8 @@ static void xhci_pci_quirks(struct device *dev, >> struct xhci_hcd *xhci) >> /* xHC spec requires PCI devices to support D3hot and D3cold */ >> if (xhci->hci_version >= 0x120) >> xhci->quirks |= XHCI_DEFAULT_PM_RUNTIME_ALLOW; >> + else if (pdev->vendor == PCI_VENDOR_ID_AMD && pdev->device == >> 0x43f7) >> + xhci->quirks |= XHCI_DEFAULT_PM_RUNTIME_ALLOW; > > This would fit better earlier in the code among the rest of the AMD > quirks. > See how this flag is set for some other hosts. Sure, I will make the necessary changes accordingly and send v3. Thanks, -- Basavaraj > > Thanks > Mathias >
diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c index b534ca9752be..1eb7a41a75d7 100644 --- a/drivers/usb/host/xhci-pci.c +++ b/drivers/usb/host/xhci-pci.c @@ -473,6 +473,8 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci) /* xHC spec requires PCI devices to support D3hot and D3cold */ if (xhci->hci_version >= 0x120) xhci->quirks |= XHCI_DEFAULT_PM_RUNTIME_ALLOW; + else if (pdev->vendor == PCI_VENDOR_ID_AMD && pdev->device == 0x43f7) + xhci->quirks |= XHCI_DEFAULT_PM_RUNTIME_ALLOW; if (xhci->quirks & XHCI_RESET_ON_RESUME) xhci_dbg_trace(xhci, trace_xhci_dbg_quirks,
The AMD USB host controller (1022:43f7) does not enter PCI D3 by default when nothing is connected. This is due to the policy introduced by 'commit a611bf473d1f ("xhci-pci: Set runtime PM as default policy on all xHC 1.2 or later devices")', which only covers 1.2 or later devices. Therefore, by default, allow RPM on the AMD USB controller [1022:43f7]. Fixes: 4baf12181509 ("xhci: Loosen RPM as default policy to cover for AMD xHC 1.1") Link: https://lore.kernel.org/all/12335218.O9o76ZdvQC@natalenko.name/ Cc: Mario Limonciello <mario.limonciello@amd.com> Cc: stable@vger.kernel.org Tested-by: Oleksandr Natalenko <oleksandr@natalenko.name> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com> --- Changes in v2: - Added Cc: stable@vger.kernel.org drivers/usb/host/xhci-pci.c | 2 ++ 1 file changed, 2 insertions(+)