[Public] > -----Original Message----- > From: Bjorn Helgaas <helgaas@xxxxxxxxxx> > Sent: Friday, September 3, 2021 3:55 PM > To: Quan, Evan <Evan.Quan@xxxxxxx> > Cc: linux-pci@xxxxxxxxxxxxxxx; bhelgaas@xxxxxxxxxx; Deucher, Alexander > <Alexander.Deucher@xxxxxxx>; stable@xxxxxxxxxxxxxxx > Subject: Re: [PATCH] PCI: Create device links for AMD integrated USB xHCI > and UCSI controllers > > On Fri, Sep 03, 2021 at 02:33:11PM +0800, Evan Quan wrote: > > Latest AMD GPUs have built-in USB xHCI and UCSI controllers. Add > > device link support for them. > > Please comment on > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.k > ernel.org%2Flinus%2F6d2e369f0d4c&data=04%7C01%7CAlexander.Deu > cher%40amd.com%7C9fa0d66e5f29424df36b08d96f14c710%7C3dd8961fe488 > 4e608e11a82d994e183d%7C0%7C0%7C637662957313172831%7CUnknown%7 > CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwi > LCJXVCI6Mn0%3D%7C1000&sdata=6IlPLWlcO7iptbTqfh71fe5wmHN7RN > 13OvScyYaWyI8%3D&reserved=0 . > > Is there something the PCI core is missing here? Or is there something that > needs to be added to ACPI or the PCI firmware spec? > > We want a generic way to discover dependencies like this. > > A quirk should not be necessary for spec-compliant devices. Quirks are an > ongoing maintenance burden, and they mean that new hardware won't > work correctly until the OS is patched to know about it. That's not what we > want. > > I expect we'll still need *this* quirk, but first I'd like to know whether there's > a plan to handle this more generically in the future. The requirement here is that all of the additional endpoints are dependencies for powering down the GPU. E.g., the audio controller and USB endpoints need to be in d3 before you put the GPU into d3, otherwise the non-GPU endpoints will be powered down as well behind their drivers' backs. On newer AMD hardware there is logic in the hardware to wait for all dependent devices to go into d3 before powering down everything or power up everything if anything enters d0, but this requires additional software setup in the GPU driver as well and older versions of the driver didn't set this up correctly, instead relying on software logic via dependencies. Earlier hardware didn't have that logic and needed software help. That said, I think all of the relevant drivers expect the hardware state to be powered down when d3 is entered and they may not handle a wake up properly if not all devices entered d3 and hence all of the devices never entered a powered down state. Alex > > When you repost this, please follow the subject line style of 6d2e369f0d4c > ("PCI: Add NVIDIA GPU multi-function power > dependencies") so similar patches look similar. > > > Cc: stable@xxxxxxxxxxxxxxx > > Signed-off-by: Evan Quan <evan.quan@xxxxxxx> > > --- > > drivers/pci/quirks.c | 9 +++++++-- > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index > > dea10d62d5b9..f0c5dd3406a1 100644 > > --- a/drivers/pci/quirks.c > > +++ b/drivers/pci/quirks.c > > @@ -5338,7 +5338,7 @@ > DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID, > > PCI_CLASS_MULTIMEDIA_HD_AUDIO, 8, > quirk_gpu_hda); > > > > /* > > - * Create device link for NVIDIA GPU with integrated USB xHCI Host > > + * Create device link for GPUs with integrated USB xHCI Host > > * controller to VGA. > > */ > > static void quirk_gpu_usb(struct pci_dev *usb) @@ -5347,9 +5347,11 @@ > > static void quirk_gpu_usb(struct pci_dev *usb) } > > DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_NVIDIA, > PCI_ANY_ID, > > PCI_CLASS_SERIAL_USB, 8, quirk_gpu_usb); > > +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_ATI, PCI_ANY_ID, > > + PCI_CLASS_SERIAL_USB, 8, quirk_gpu_usb); > > > > /* > > - * Create device link for NVIDIA GPU with integrated Type-C UCSI > > controller > > + * Create device link for GPUs with integrated Type-C UCSI controller > > * to VGA. Currently there is no class code defined for UCSI device over PCI > > * so using UNKNOWN class for now and it will be updated when UCSI > > * over PCI gets a class code. > > @@ -5362,6 +5364,9 @@ static void quirk_gpu_usb_typec_ucsi(struct > > pci_dev *ucsi) > DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID, > > PCI_CLASS_SERIAL_UNKNOWN, 8, > > quirk_gpu_usb_typec_ucsi); > > +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_ATI, PCI_ANY_ID, > > + PCI_CLASS_SERIAL_UNKNOWN, 8, > > + quirk_gpu_usb_typec_ucsi); > > > > /* > > * Enable the NVIDIA GPU integrated HDA controller if the BIOS left > > it > > -- > > 2.29.0 > >
