Re: [PATCH] PCI: Only put >= 2015 root ports into D3 on Intel

["Limonciello, Mario" <mario.limonciello@amd.com>]

On 5/17/2023 8:48 PM, Limonciello, Mario wrote:
>
> On 5/17/2023 7:58 AM, Mika Westerberg wrote:
>> On Wed, May 17, 2023 at 07:33:17AM -0500, Limonciello, Mario wrote:
>>>>> AFAICT the actual issue is entirely a wakeup platform firmware 
>>>>> sequencing
>>>>> issue
>>>>> while in a hardware sleep state and not PMEs.
>>>>>
>>>>> It's only exposed by putting the root ports into D3 over s2idle.
>>>> But there are two ways to enter s2idle (well or the S0ix whatever 
>>>> is the
>>>> AMD term for that). Either through system sleep or simply waiting that
>>>> all the needed devices runtime suspend. There should be no difference
>>>> from device perspective AFAICT.
>> I should correct that the wakes may be configured differently, though.
>>
>>> On AMD all devices in runtime suspend and SoC entering system
>>> suspend aren't the same state.
>> Okay.
>>
>>>>> As an experiment on an unpatched kernel if I avoid letting amd-pmc 
>>>>> bind then
>>>>> the
>>>>> hardware will never enter a hardware sleep state over Linux s2idle 
>>>>> and this
>>>>> issue
>>>>> doesn't occur.
>>>>>
>>>>> That shows that PMEs *do* work from D3cold.
>>>>>
>>>>> With all of this I have to wonder if the Windows behavior of what 
>>>>> to do with
>>>>> the root
>>>>> ports is tied to the uPEP requirements specified in the firmware.
>>>>>
>>>>> Linux doesn't do any enforcement or adjustments from what uPEP 
>>>>> indicates.
>>>>>
>>>>> The uPEP constraints for the root port in question in an affected 
>>>>> AMD system
>>>>> has:
>>>>>
>>>>>                       Package (0x04)
>>>>>                       {
>>>>>                           Zero,
>>>>>                           "\\_SB.PCI0.GP19",
>>>>>                           Zero,
>>>>>                           Zero
>>>>>                       },
>>>>>
>>>>> AMD's parsing is through 'lpi_device_get_constraints_amd' so that 
>>>>> structure
>>>>> shows
>>>>> as not enabled and doesn't specify any D-state requirements.
>>>> AFAIK this object does not exist in ChromeOS so Linux cannot use it
>>>> there.
>>> OK that means that if we came up with a solution that utilized
>>> uPEP that it would have to remain optional.
>> Right.
>>
>>>>> What do they specify for Intel on a matching root port?
>>>> I think the corresponding entry in ADL-P system for TBT PCIe root 
>>>> port 0
>>>> looks like this:
>>>>
>>>>     Package (0x03)
>>>>     {
>>>>         "\\_SB.PC00.TRP0",
>>>>         Zero,
>>>>         Package (0x02)
>>>>         {
>>>>         Zero,
>>>>         Package (0x02)
>>>>         {
>>>>             0xFF,
>>>>             0x03
>>>>         }
>>>>         }
>>>>     },
>>>>
>>>> I'm not entirely sure what does it tell? ;-)
>>> It's parsed using `lpi_device_get_constraints`.
>>>
>>> So should I follow it right this means for ACPI device
>>> \\_SB.PC00.TRP0 the constraint is disabled.
>>>
>>> It's described as
>>> Version 0, UID 0xFF has a minimum D-state of 3.
>> I see, so it needs to be in D3 for this "constraint" to be valid.
>>
>>> That means my idea to try to only change D-states at
>>> suspend for enabled constraints won't help.
>> :(
>
> At least on an Alder Lake P system can you check
> whether your root ports actually respond
> affirmatively to acpi_pci_bridge_d3() or they need
> to fall back to that logic?
>
> In my case the problematic ones don't have _PRW or
> _S0W, which might explain why Windows doesn't try
> to use D3 (hot or cold) for them either.
>
> If the root ports on your current systems do respond
> through acpi_pci_bridge_d3() a possible solution here
> might be an allow list for systems from 2015-2018 rather
> than everything > 2015.
>
Actually this is exactly it.  The function is missing a call

for platform_pci_power_manageable().

I'll send out a v2.