RE: [PATCH] PCI: Create device links for AMD integrated USB xHCI and UCSI controllers

From: "Deucher, Alexander" <Alexander.Deucher@amd.com>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: "Quan, Evan" <Evan.Quan@amd.com>,
	"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
	"bhelgaas@google.com" <bhelgaas@google.com>,
	"stable@vger.kernel.org" <stable@vger.kernel.org>,
	"Rafael J. Wysocki" <rjw@rjwysocki.net>
Subject: RE: [PATCH] PCI: Create device links for AMD integrated USB xHCI and UCSI controllers
Date: Tue, 14 Sep 2021 12:59:08 +0000	[thread overview]
Message-ID: <BL1PR12MB514448B91AEAEAADB521F427F7DA9@BL1PR12MB5144.namprd12.prod.outlook.com> (raw)
In-Reply-To: <20210907173513.GA746796@bjorn-Precision-5520>

[Public]

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: Tuesday, September 7, 2021 1:35 PM
> To: Deucher, Alexander <Alexander.Deucher@amd.com>
> Cc: Quan, Evan <Evan.Quan@amd.com>; linux-pci@vger.kernel.org;
> bhelgaas@google.com; stable@vger.kernel.org; Rafael J. Wysocki
> <rjw@rjwysocki.net>
> Subject: Re: [PATCH] PCI: Create device links for AMD integrated USB xHCI
> and UCSI controllers
> 
> [+cc Rafael, beginning of thread:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.
> kernel.org%2Fall%2F20210903063311.3606226-1-
> evan.quan%40amd.com%2F&amp;data=04%7C01%7CAlexander.Deucher%4
> 0amd.com%7C48f3a6f7639343fcad6208d97225da95%7C3dd8961fe4884e608e
> 11a82d994e183d%7C0%7C0%7C637666329177869121%7CUnknown%7CTWFp
> bGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVC
> I6Mn0%3D%7C1000&amp;sdata=k%2FgSHhtLiS8xS5hNkaP%2BOWWMWqiM
> 4tgQk4bybfumvfM%3D&amp;reserved=0]
> 
> On Tue, Sep 07, 2021 at 04:09:40PM +0000, Deucher, Alexander wrote:
> > > -----Original Message-----
> > > From: Bjorn Helgaas <helgaas@kernel.org>
> > > Sent: Friday, September 3, 2021 3:55 PM
> > > To: Quan, Evan <Evan.Quan@amd.com>
> > > Cc: linux-pci@vger.kernel.org; bhelgaas@google.com; Deucher,
> > > Alexander <Alexander.Deucher@amd.com>; stable@vger.kernel.org
> > > 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%2Fgi
> > >
> t.k%2F&amp;data=04%7C01%7CAlexander.Deucher%40amd.com%7C48f3a6f
> 76393
> > >
> 43fcad6208d97225da95%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0
> %7C63
> > >
> 7666329177869121%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMD
> AiLCJQIjo
> > >
> iV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=0yxZbS6o
> P56
> > > rXpA5j1wvlMpkp9Ern%2FcSRCPELtv47lM%3D&amp;reserved=0
> > >
> ernel.org%2Flinus%2F6d2e369f0d4c&amp;data=04%7C01%7CAlexander.Deu
> > >
> cher%40amd.com%7C9fa0d66e5f29424df36b08d96f14c710%7C3dd8961fe488
> > >
> 4e608e11a82d994e183d%7C0%7C0%7C637662957313172831%7CUnknown%7
> > >
> CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwi
> > >
> LCJXVCI6Mn0%3D%7C1000&amp;sdata=6IlPLWlcO7iptbTqfh71fe5wmHN7RN
> > > 13OvScyYaWyI8%3D&amp;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.
> 
> I'm not sure whether this answered my question.  Will we need more quirks
> for future devices?

The existing quirks should cover all devices unless we add some new endpoint to the GPUs in the future.  The quirk for the audio dependency was added years ago and hasn't needed to be extended since.  The USB stuff was added more recently and requires adding a similar quirk.

> 
> You said "On newer AMD hardware there is logic to wait for all dependent
> devices to go to d3 ...," which sounds promising, but then it "requires
> additional setup in the GPU driver."
> 
> So maybe PM works as per PCIe spec, but only after the driver sets things
> up?  I'm not sure what, if any, PM we do before a driver claims the device.
> 

I'm not exactly sure what the expectation is with regards to the spec.  There is a single power rail that controls all of the endpoints so all have to be in d3 before the power can be cut.

> The above suggests that if we put some (but not all) functions, in D3, the
> new logic will keep them from entering D3 until later.  That doesn't really
> *sound* spec-compliant -- if we write D3 to a function's PCI_PM_CTRL and
> then read it back, the function will remain in D0 indefinitely, until we put that
> last function in D3?
> 

It will read back as if the card is in D3, but the power rail stays on until all devices have been put into D3.

> pci_raw_set_power_state() does this read then write, and it expects
> PCI_PM_CTRL to change to the new state after the delay prescribed by the
> spec, which of course has nothing to do with sibling functions.
> 
> And if all the functions are in D3, and we write D0 to one function's
> PCI_PM_CTRL, *all* the functions magically go to D0?  That sounds
> potentially confusing.

The will all individually report the correct D state, it's just that they will be using more power than if they were all in D3.

Alex

> 
> Bjorn