Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 2 Aug 2023 17:31:42 +0300
From:   Mika Westerberg <mika.westerberg@linux.intel.com>
To:     Mario Limonciello <mario.limonciello@amd.com>
Cc:     "Rafael J. Wysocki" <rafael@kernel.org>,
        Bjorn Helgaas <helgaas@kernel.org>, linux-pci@vger.kernel.org,
        linux-kernel@vger.kernel.org, Len Brown <lenb@kernel.org>,
        linux-acpi@vger.kernel.org, Iain Lane <iain@orangesquash.org.uk>,
        Kuppuswamy Sathyanarayanan 
        <sathyanarayanan.kuppuswamy@linux.intel.com>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Subject: Re: [PATCH v7 2/2] PCI: Don't put non-power manageable PCIe root
 ports into D3
Message-ID: <20230802143142.GS14638@black.fi.intel.com>
References: <b82a50eb-8182-84ca-5b24-dbe8870fa871@amd.com>
 <CAJZ5v0i6PviqW7u3i8hmvSCvR_VHqP-mWRy3Da8Ev_1vi9qBQA@mail.gmail.com>
 <a309e3fe-b1f9-e269-cb97-8af87c8d483b@amd.com>
 <CAJZ5v0jvxrDMR6YHFpYZ4yYpp82-3TtrH==SMRFtUMJsv7=i=g@mail.gmail.com>
 <37b005d5-68fb-f8dd-67e2-c953d677fca2@amd.com>
 <8298c01c-abec-914b-0542-459f38c635fe@amd.com>
 <CAJZ5v0i3g0JujMwikB8niRZ93hXJZqWtjrCjbaDmkMLUbMmwMA@mail.gmail.com>
 <d1b2cf1b-de5f-6c2e-c8dc-fdf60cd0882d@amd.com>
 <20230802052601.GM14638@black.fi.intel.com>
 <e82ec662-22d9-b331-0880-886bd28624eb@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <e82ec662-22d9-b331-0880-886bd28624eb@amd.com>
Precedence: bulk

Hi,

On Wed, Aug 02, 2023 at 09:10:38AM -0500, Mario Limonciello wrote:
> 
> 
> On 8/2/23 00:26, Mika Westerberg wrote:
> > Hi Mario,
> > 
> > On Tue, Aug 01, 2023 at 10:17:11PM -0500, Mario Limonciello wrote:
> > > > Consequently, platform_pci_bridge_d3() will return false and the only
> > > > thing that may allow the port to go into D0 is the dmi_get_bios_year()
> > > > check at the end of pci_bridge_d3_possible().
> > > > 
> > > > However, that was added, because there are Intel platforms on which
> > > > Root Ports need to be programmed into D3hot on suspend (which allows
> > > > the whole platform to reduce power significantly) and there are no
> > > > ACPI device power management objects associated with them (Mika should
> > > > know the gory details related to this).  It looks like under Windows
> > > > the additional power reduction would not be possible on those systems,
> > > > but that would be a problem, wouldn't it?
> > > > 
> > > 
> > > I've been thinking on this today, and I at least have a hypothesis about
> > > this behavior.  Perhaps Windows is actually utilizing enabled PEP
> > > constraints to enforce what state device should be put into over Modern
> > > Standby cycles in the absence of ACPI objects.
> > > 
> > > In the case of one of my problematic system the PEP constraints for the root
> > > port are:
> > > 
> > > Package (0x04)
> > > {
> > > 	0x00,
> > > 	"\\_SB.PCI0.GP17",
> > > 	0x00,
> > > 	0x00
> > > },
> > > 
> > > That first 0x00 means the constraint isn't actually enabled for the root
> > > port.
> > > 
> > > Mika,
> > > 
> > > Could you get an acpidump from one of these problematic Intel systems so we
> > > can check the PEP constraints to see if this theory works? Or maybe you have
> > > some other ideas why this is different?
> > 
> > The patch adding this was merged in 2016 and unfortunately I don't have
> > any of the ACPI dumps from them available anymore (and do not recall the
> > details either). I think these were Apollo Lake-P based systems with the
> > initial runtime D3cold and S0ix support at the time.
> 
> 
> I scoured the web looking for acpidumps a bit an Apollo Lake system and came
> across this random bug report:
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1591307
> 
> "Intel(R) Celeron(R) CPU N3450 @ 1.10GHz (family: 0x6, model: 0x5c,
> stepping: 0x9)"
> 
> I looked at the acpidump, and I notice:
> 
> Low Power S0 Idle (V5) : 0
> 
> That means that Windows wouldn't actually be putting it into Modern Standby
> at suspend but would rather use S3.

Same goes for Linux AFAICT. The ones needed this actually used S0ix so
the bit should definitely be set.

> Considering that result, could we perhaps adjust the check to:
> 
> if ((c->x86_vendor == X86_VENDOR_INTEL) && !(acpi_gbl_FADT.flags &
> ACPI_FADT_LOW_POWER_S0))
> 
> Or could we quirk the PCI root ports from Apollo Lake to opt into D3?

It is not just Apollo Lake, but all "modern" systems as well (sorry if
this was unclear). Apollo Lake just was the first one that needed this.
We also have the Low Power S0 Idle bit set in recent systems too.