My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

[Kenneth R. Crudup]

[-- Attachment #1: Type: text/plain, Size: 2524 bytes --]


I have a Dell XPS-9320 with an Alderlake chipset, and the NVMe behind a
VMD device:

----
[    0.127342] smpboot: CPU0: 12th Gen Intel(R) Core(TM) i7-1280P (family: 0x6, model: 0x9a, stepping: 0x3)
----
0000:00:0e.0 0104: 8086:467f
        Subsystem: 1028:0af3
        Flags: bus master, fast devsel, latency 0, IOMMU group 9
        Memory at 603c000000 (64-bit, non-prefetchable) [size=32M]
        Memory at 72000000 (32-bit, non-prefetchable) [size=32M]
a7152be79b6        Memory at 6040100000 (64-bit, non-prefetchable) [size=1M]
        Capabilities: <access denied>
        Kernel driver in use: vmd
----

The only release kernel that was able to get this laptop to fully get into
low-power (unfortunately only s0ix) was the Ubuntu-6.2.0- ... series from
Ubuntu
(remote git://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/lunar).

I'd bisected it to the following commits (in this order):

4ff116d0d5fd PCI/ASPM: Save L1 PM Substates Capability for suspend/resume
5e85eba6f50d PCI/ASPM: Refactor L1 PM Substates Control Register programming
1a0102a08f20 UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under VMD domain
47c7bfd31514 UBUNTU: SAUCE: PCI/ASPM: Enable LTR for endpoints behind VMD
154d48da2c57 UBUNTU: SAUCE: vmd: fixup bridge ASPM by driver name instead

Without the patches I never see Pkg%PC8 or higher(? lower?), nor i915 states
DC5/6, all necssary for SYS%LPI/CPU%LPI. I've attached a little script I use
alongside turbostat for verifying low-power operation (and also for seeing
what chipset subsystem may be preventing it).

The first two are in Linus' trees, but were reverted (4ff116d0d5fd in
a7152be79b6, 5e85eba6f50d in ff209ecc376a). The last three come from Ubuntu's
Linux trees (see remote spec above). The first two remain reverted in the
Ubuntu trees, but if I put them back, I get increased power savings during
suspend/resume cycles.

Considering the power draw is really significant without these patches (10s
of %s per hour) and I'd think Dell would have sold some decent number of
these laptops, I'd been patiently waiting for these patches, or some variant
to show up in the stable trees, but so far I'm up to the 6.6 stable kernel
and still having to manually cherry-pick these, so I thought maybe I could
bring this to the PM maintainers' attention so at least start a discussion
about this issue.

Apologies about the Maintainer Spam, and if this is already being discussed.

	-Kenny

-- 
Kenneth R. Crudup / Sr. SW Engineer, Scott County Consulting, Orange County CA

[-- Attachment #2: Type: text/plain, Size: 777 bytes --]

#!/bin/bash -e
date
egrep -Hr . /sys/class/drm/card0/power/rc6_residency_ms \
 /sys/devices/system/cpu/cpuidle/low_power_idle_system_residency_us \
 /sys/kernel/debug/pmc_core/package_cstate_show \
 /sys/kernel/debug/pmc_core/slp_s0_residency_usec \
 /sys/kernel/debug/dri/0/i915_edp_psr_status \
 /sys/kernel/debug/dri/0/i915_dmc_info | tee -a ~/Dropbox/XPS-7390/sleep-params
egrep '\(ns\): [^0]' /sys/kernel/debug/pmc_core/ltr_show | cut -d'	' -f1,3,4 | sed -e 's;[	][	]*; ;' | tee -a ~/Dropbox/XPS-7390/sleep-params
egrep -Hr ": On" /sys/kernel/debug/pmc_core/pch_ip_power_gating_status | tee -a /dev/tty | tee -a ~/Dropbox/XPS-7390/sleep-params | wc -l
egrep No /sys/kernel/debug/pmc_core/slp_s0_debug_status 2>/dev/null | tee -a ~/Dropbox/XPS-7390/sleep-params

Re: My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

[Bjorn Helgaas]

[+cc Mika, Sathy, Rafael, David, Ilpo, Ricky, Mario, linux-pci]

On Sat, Nov 04, 2023 at 10:13:24AM -0700, Kenneth R. Crudup wrote:
> 
> I have a Dell XPS-9320 with an Alderlake chipset, and the NVMe behind a
> VMD device:
> 
> ----
> [    0.127342] smpboot: CPU0: 12th Gen Intel(R) Core(TM) i7-1280P (family: 0x6, model: 0x9a, stepping: 0x3)
> ----
> 0000:00:0e.0 0104: 8086:467f
>         Subsystem: 1028:0af3
>         Flags: bus master, fast devsel, latency 0, IOMMU group 9
>         Memory at 603c000000 (64-bit, non-prefetchable) [size=32M]
>         Memory at 72000000 (32-bit, non-prefetchable) [size=32M]
> a7152be79b6        Memory at 6040100000 (64-bit, non-prefetchable) [size=1M]
>         Capabilities: <access denied>
>         Kernel driver in use: vmd
> ----
> 
> The only release kernel that was able to get this laptop to fully get into
> low-power (unfortunately only s0ix) was the Ubuntu-6.2.0- ... series from
> Ubuntu
> (remote git://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/lunar).
> 
> I'd bisected it to the following commits (in this order):
> 
> 4ff116d0d5fd PCI/ASPM: Save L1 PM Substates Capability for suspend/resume
> 5e85eba6f50d PCI/ASPM: Refactor L1 PM Substates Control Register programming
> 1a0102a08f20 UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under VMD domain
> 47c7bfd31514 UBUNTU: SAUCE: PCI/ASPM: Enable LTR for endpoints behind VMD
> 154d48da2c57 UBUNTU: SAUCE: vmd: fixup bridge ASPM by driver name instead

Thanks for these.  You don't happen to have URLs for those Ubuntu
commits, do you?  E.g., https://git.kernel.org/linus/4ff116d0d5fd
(which was reverted by a7152be79b62 ("Revert "PCI/ASPM: Save L1 PM
Substates Capability for suspend/resume"")).

> Without the patches I never see Pkg%PC8 or higher(? lower?), nor i915 states
> DC5/6, all necssary for SYS%LPI/CPU%LPI. I've attached a little script I use
> alongside turbostat for verifying low-power operation (and also for seeing
> what chipset subsystem may be preventing it).
> 
> The first two are in Linus' trees, but were reverted (4ff116d0d5fd in
> a7152be79b6, 5e85eba6f50d in ff209ecc376a). The last three come from Ubuntu's
> Linux trees (see remote spec above). The first two remain reverted in the
> Ubuntu trees, but if I put them back, I get increased power savings during
> suspend/resume cycles.
> 
> Considering the power draw is really significant without these patches (10s
> of %s per hour) and I'd think Dell would have sold some decent number of
> these laptops, I'd been patiently waiting for these patches, or some variant
> to show up in the stable trees, but so far I'm up to the 6.6 stable kernel
> and still having to manually cherry-pick these, so I thought maybe I could
> bring this to the PM maintainers' attention so at least start a discussion
> about this issue.

Thank you very much for raising this again.  We really need to make
some progress, and Mika recently posted a patch to add the
4ff116d0d5fd functionality again:
https://lore.kernel.org/r/20231002070044.2299644-1-mika.westerberg@linux.intel.com

The big problem is that it works on *most* systems, but it still seems
to break a few.  So Mika's current patch relies on a denylist of
systems where we *don't* restore the substates.

It's possible we'll have to give in and use a denylist, but it's
obviously not ideal because (a) we don't know *why* it doesn't work on
those systems, and (b) it means substates work before suspend but not
after resume, which is a poor user experience.

Bjorn

> #!/bin/bash -e
> date
> egrep -Hr . /sys/class/drm/card0/power/rc6_residency_ms \
>  /sys/devices/system/cpu/cpuidle/low_power_idle_system_residency_us \
>  /sys/kernel/debug/pmc_core/package_cstate_show \
>  /sys/kernel/debug/pmc_core/slp_s0_residency_usec \
>  /sys/kernel/debug/dri/0/i915_edp_psr_status \
>  /sys/kernel/debug/dri/0/i915_dmc_info | tee -a ~/Dropbox/XPS-7390/sleep-params
> egrep '\(ns\): [^0]' /sys/kernel/debug/pmc_core/ltr_show | cut -d'	' -f1,3,4 | sed -e 's;[	][	]*; ;' | tee -a ~/Dropbox/XPS-7390/sleep-params
> egrep -Hr ": On" /sys/kernel/debug/pmc_core/pch_ip_power_gating_status | tee -a /dev/tty | tee -a ~/Dropbox/XPS-7390/sleep-params | wc -l
> egrep No /sys/kernel/debug/pmc_core/slp_s0_debug_status 2>/dev/null | tee -a ~/Dropbox/XPS-7390/sleep-params

Re: My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

[Mika Westerberg]

Hi,

On Mon, Nov 06, 2023 at 12:11:07PM -0600, Bjorn Helgaas wrote:
> [+cc Mika, Sathy, Rafael, David, Ilpo, Ricky, Mario, linux-pci]
> 
> On Sat, Nov 04, 2023 at 10:13:24AM -0700, Kenneth R. Crudup wrote:
> > 
> > I have a Dell XPS-9320 with an Alderlake chipset, and the NVMe behind a
> > VMD device:
> > 
> > ----
> > [    0.127342] smpboot: CPU0: 12th Gen Intel(R) Core(TM) i7-1280P (family: 0x6, model: 0x9a, stepping: 0x3)
> > ----
> > 0000:00:0e.0 0104: 8086:467f
> >         Subsystem: 1028:0af3
> >         Flags: bus master, fast devsel, latency 0, IOMMU group 9
> >         Memory at 603c000000 (64-bit, non-prefetchable) [size=32M]
> >         Memory at 72000000 (32-bit, non-prefetchable) [size=32M]
> > a7152be79b6        Memory at 6040100000 (64-bit, non-prefetchable) [size=1M]
> >         Capabilities: <access denied>
> >         Kernel driver in use: vmd
> > ----
> > 
> > The only release kernel that was able to get this laptop to fully get into
> > low-power (unfortunately only s0ix) was the Ubuntu-6.2.0- ... series from
> > Ubuntu
> > (remote git://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/lunar).
> > 
> > I'd bisected it to the following commits (in this order):
> > 
> > 4ff116d0d5fd PCI/ASPM: Save L1 PM Substates Capability for suspend/resume
> > 5e85eba6f50d PCI/ASPM: Refactor L1 PM Substates Control Register programming
> > 1a0102a08f20 UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under VMD domain
> > 47c7bfd31514 UBUNTU: SAUCE: PCI/ASPM: Enable LTR for endpoints behind VMD
> > 154d48da2c57 UBUNTU: SAUCE: vmd: fixup bridge ASPM by driver name instead
> 
> Thanks for these.  You don't happen to have URLs for those Ubuntu
> commits, do you?  E.g., https://git.kernel.org/linus/4ff116d0d5fd
> (which was reverted by a7152be79b62 ("Revert "PCI/ASPM: Save L1 PM
> Substates Capability for suspend/resume"")).
> 
> > Without the patches I never see Pkg%PC8 or higher(? lower?), nor i915 states
> > DC5/6, all necssary for SYS%LPI/CPU%LPI. I've attached a little script I use
> > alongside turbostat for verifying low-power operation (and also for seeing
> > what chipset subsystem may be preventing it).
> > 
> > The first two are in Linus' trees, but were reverted (4ff116d0d5fd in
> > a7152be79b6, 5e85eba6f50d in ff209ecc376a). The last three come from Ubuntu's
> > Linux trees (see remote spec above). The first two remain reverted in the
> > Ubuntu trees, but if I put them back, I get increased power savings during
> > suspend/resume cycles.
> > 
> > Considering the power draw is really significant without these patches (10s
> > of %s per hour) and I'd think Dell would have sold some decent number of
> > these laptops, I'd been patiently waiting for these patches, or some variant
> > to show up in the stable trees, but so far I'm up to the 6.6 stable kernel
> > and still having to manually cherry-pick these, so I thought maybe I could
> > bring this to the PM maintainers' attention so at least start a discussion
> > about this issue.
> 
> Thank you very much for raising this again.  We really need to make
> some progress, and Mika recently posted a patch to add the
> 4ff116d0d5fd functionality again:
> https://lore.kernel.org/r/20231002070044.2299644-1-mika.westerberg@linux.intel.com
> 
> The big problem is that it works on *most* systems, but it still seems
> to break a few.  So Mika's current patch relies on a denylist of
> systems where we *don't* restore the substates.

According to latest reports it is just that one system where this is
still an issue. The latest patch works in Asus UX305FA even if it is not
in the denylist. That would leave that one system only to the denylist,
at least the ones we are aware about.

Re: My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

[Kai-Heng Feng]

Hi Kenneth,

On Sat, Nov 4, 2023 at 7:13 PM Kenneth R. Crudup <kenny@panix.com> wrote:
>
>
> I have a Dell XPS-9320 with an Alderlake chipset, and the NVMe behind a
> VMD device:
>
> ----
> [    0.127342] smpboot: CPU0: 12th Gen Intel(R) Core(TM) i7-1280P (family: 0x6, model: 0x9a, stepping: 0x3)
> ----
> 0000:00:0e.0 0104: 8086:467f
>         Subsystem: 1028:0af3
>         Flags: bus master, fast devsel, latency 0, IOMMU group 9
>         Memory at 603c000000 (64-bit, non-prefetchable) [size=32M]
>         Memory at 72000000 (32-bit, non-prefetchable) [size=32M]
> a7152be79b6        Memory at 6040100000 (64-bit, non-prefetchable) [size=1M]
>         Capabilities: <access denied>
>         Kernel driver in use: vmd
> ----
>
> The only release kernel that was able to get this laptop to fully get into
> low-power (unfortunately only s0ix) was the Ubuntu-6.2.0- ... series from
> Ubuntu
> (remote git://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/lunar).
>
> I'd bisected it to the following commits (in this order):
>
> 4ff116d0d5fd PCI/ASPM: Save L1 PM Substates Capability for suspend/resume
> 5e85eba6f50d PCI/ASPM: Refactor L1 PM Substates Control Register programming
> 1a0102a08f20 UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under VMD domain
> 47c7bfd31514 UBUNTU: SAUCE: PCI/ASPM: Enable LTR for endpoints behind VMD
> 154d48da2c57 UBUNTU: SAUCE: vmd: fixup bridge ASPM by driver name instead
>
> Without the patches I never see Pkg%PC8 or higher(? lower?), nor i915 states
> DC5/6, all necssary for SYS%LPI/CPU%LPI. I've attached a little script I use
> alongside turbostat for verifying low-power operation (and also for seeing
> what chipset subsystem may be preventing it).
>
> The first two are in Linus' trees, but were reverted (4ff116d0d5fd in
> a7152be79b6, 5e85eba6f50d in ff209ecc376a). The last three come from Ubuntu's
> Linux trees (see remote spec above). The first two remain reverted in the
> Ubuntu trees, but if I put them back, I get increased power savings during
> suspend/resume cycles.

I am working on this, hopefully I can come up with an upstream worthy
patch soon.

Kai-Heng

>
> Considering the power draw is really significant without these patches (10s
> of %s per hour) and I'd think Dell would have sold some decent number of
> these laptops, I'd been patiently waiting for these patches, or some variant
> to show up in the stable trees, but so far I'm up to the 6.6 stable kernel
> and still having to manually cherry-pick these, so I thought maybe I could
> bring this to the PM maintainers' attention so at least start a discussion
> about this issue.
>
> Apologies about the Maintainer Spam, and if this is already being discussed.
>
>         -Kenny
>
> --
> Kenneth R. Crudup / Sr. SW Engineer, Scott County Consulting, Orange County CA

Re: My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

[Kenneth R. Crudup]

On Mon, 6 Nov 2023, Bjorn Helgaas wrote:

> > I'd bisected it to the following commits (in this order):
> > 4ff116d0d5fd PCI/ASPM: Save L1 PM Substates Capability for suspend/resume
> > 5e85eba6f50d PCI/ASPM: Refactor L1 PM Substates Control Register programming
> > 1a0102a08f20 UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under VMD domain
> > 47c7bfd31514 UBUNTU: SAUCE: PCI/ASPM: Enable LTR for endpoints behind VMD
> > 154d48da2c57 UBUNTU: SAUCE: vmd: fixup bridge ASPM by driver name instead

> Thanks for these.  You don't happen to have URLs for those Ubuntu
> commits, do you?

https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/lunar/commit/?id=1a0102a08f206149d9abd56c2b28877c878b5526
https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/lunar/commit/?id=47c7bfd31514e7b54a1f830f7707297aebbb8679
https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/lunar/commit/?id=154d48da2c57514e4b5dadc7b8c70a4edb550981

> Thank you very much for raising this again.

They've been really great for battery life on my laptop, so I'd like to help
these in some form get upstreamed (provided there's no bad side-effects, of
course).

	-Kenny

-- 
Kenneth R. Crudup / Sr. SW Engineer, Scott County Consulting, Orange County CA

Re: My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

[Kenneth R. Crudup]

On Wed, 8 Nov 2023, Kai-Heng Feng wrote:

> I am working on this, hopefully I can come up with an upstream worthy
> patch soon.

Thanks, everyone. Let me know if there's anything I can/should do to help.

	-Kenny

-- 
Kenneth R. Crudup / Sr. SW Engineer, Scott County Consulting, Orange County CA

Re: My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

[David E. Box]

Hi Mika, Bjorn,

On Tue, 2023-11-07 at 13:15 +0200, Mika Westerberg wrote:
> Hi,
> 
> On Mon, Nov 06, 2023 at 12:11:07PM -0600, Bjorn Helgaas wrote:
> > [+cc Mika, Sathy, Rafael, David, Ilpo, Ricky, Mario, linux-pci]
> > 
> > On Sat, Nov 04, 2023 at 10:13:24AM -0700, Kenneth R. Crudup wrote:
> > > 
> > > I have a Dell XPS-9320 with an Alderlake chipset, and the NVMe behind a
> > > VMD device:
> > > 
> > > ----
> > > [    0.127342] smpboot: CPU0: 12th Gen Intel(R) Core(TM) i7-1280P (family:
> > > 0x6, model: 0x9a, stepping: 0x3)
> > > ----
> > > 0000:00:0e.0 0104: 8086:467f
> > >         Subsystem: 1028:0af3
> > >         Flags: bus master, fast devsel, latency 0, IOMMU group 9
> > >         Memory at 603c000000 (64-bit, non-prefetchable) [size=32M]
> > >         Memory at 72000000 (32-bit, non-prefetchable) [size=32M]
> > > a7152be79b6        Memory at 6040100000 (64-bit, non-prefetchable)
> > > [size=1M]
> > >         Capabilities: <access denied>
> > >         Kernel driver in use: vmd
> > > ----
> > > 
> > > The only release kernel that was able to get this laptop to fully get into
> > > low-power (unfortunately only s0ix) was the Ubuntu-6.2.0- ... series from
> > > Ubuntu
> > > (remote git://git.launchpad.net/~ubuntu-
> > > kernel/ubuntu/+source/linux/+git/lunar).
> > > 
> > > I'd bisected it to the following commits (in this order):
> > > 
> > > 4ff116d0d5fd PCI/ASPM: Save L1 PM Substates Capability for suspend/resume
> > > 5e85eba6f50d PCI/ASPM: Refactor L1 PM Substates Control Register
> > > programming
> > > 1a0102a08f20 UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under VMD
> > > domain
> > > 47c7bfd31514 UBUNTU: SAUCE: PCI/ASPM: Enable LTR for endpoints behind VMD
> > > 154d48da2c57 UBUNTU: SAUCE: vmd: fixup bridge ASPM by driver name instead
> > 
> > Thanks for these.  You don't happen to have URLs for those Ubuntu
> > commits, do you?  E.g., https://git.kernel.org/linus/4ff116d0d5fd
> > (which was reverted by a7152be79b62 ("Revert "PCI/ASPM: Save L1 PM
> > Substates Capability for suspend/resume"")).
> > 
> > > Without the patches I never see Pkg%PC8 or higher(? lower?), nor i915
> > > states
> > > DC5/6, all necssary for SYS%LPI/CPU%LPI. I've attached a little script I
> > > use
> > > alongside turbostat for verifying low-power operation (and also for seeing
> > > what chipset subsystem may be preventing it).
> > > 
> > > The first two are in Linus' trees, but were reverted (4ff116d0d5fd in
> > > a7152be79b6, 5e85eba6f50d in ff209ecc376a). The last three come from
> > > Ubuntu's
> > > Linux trees (see remote spec above). The first two remain reverted in the
> > > Ubuntu trees, but if I put them back, I get increased power savings during
> > > suspend/resume cycles.
> > > 
> > > Considering the power draw is really significant without these patches
> > > (10s
> > > of %s per hour) and I'd think Dell would have sold some decent number of
> > > these laptops, I'd been patiently waiting for these patches, or some
> > > variant
> > > to show up in the stable trees, but so far I'm up to the 6.6 stable kernel
> > > and still having to manually cherry-pick these, so I thought maybe I could
> > > bring this to the PM maintainers' attention so at least start a discussion
> > > about this issue.
> > 
> > Thank you very much for raising this again.  We really need to make
> > some progress, and Mika recently posted a patch to add the
> > 4ff116d0d5fd functionality again:
> > https://lore.kernel.org/r/20231002070044.2299644-1-mika.westerberg@linux.intel.com
> > 
> > The big problem is that it works on *most* systems, but it still seems
> > to break a few.  So Mika's current patch relies on a denylist of
> > systems where we *don't* restore the substates.
> 
> According to latest reports it is just that one system where this is
> still an issue. The latest patch works in Asus UX305FA even if it is not
> in the denylist. That would leave that one system only to the denylist,
> at least the ones we are aware about.

I've been working with Thomas, whose system is the last known to have problems
with Mika's patch. It turns out that his config sets aspm_policy to 'powersave'.
If he sets it to any other policy, Mika's patch works [1]. It's possible that
others may see the same issue if they use 'powersave' as well.

The theory right now is that enabling L1SS in pci_restore_state() is too early.
During boot, if ASPM policy is 'powersave' or 'powersupersave', ASPM enabling is
deferred. The comment in pcie_aspm_init_link_state() that skips it state that:

        /*
         * At this stage drivers haven't had an opportunity to change the
         * link policy setting. Enabling ASPM on broken hardware can cripple
         * it even before the driver has had a chance to disable ASPM, so
         * default to a safe level right now. If we're enabling ASPM beyond
         * the BIOS's expectation, we'll do so once pci_enable_device() is
         * called.
         */

While pci_enable_device() is called by the PCI core before pci_restore_state()
on resume, it is called again later by the nvme driver in nvme_pci_enable().
This stage seems the intended intercept mentioned in the comment. This ends up
calling pcie_aspm_powersave_config_link() to configure ASPM at that time. During
boot we see ASPM enabling is indeed happening for powersave during
nvme_pci_enable(). With the save/restore patch however it is being restored
before nvme_pci_enable(). I've asked Thomas not to apply Mika's patch, but
instead use a different patch [2] that waits until
pcie_aspm_powersave_config_link() is called to configure ASPM. The need for this
is mentioned below. Hopefully it will fix the hang observed on his system.

Whether that patch works for him, we can address his problem with the current
L1SS save/restore patch by removing the current denylist and instead only do the
save/restore if ASPM policy is 'default' which doesn't hang his system. This
makes sense since it's only the BIOS config that we care to preserve since it
can be lost during suspend, particularly during s2idle. All other policies are
OS controlled if allowed. Instead of save/restore for those we can let it be
configured later when pcie_aspm_powersave_config_link() is called.

The only issue with this is that pcie_aspm_powersave_config_link() will not
configure ASPM if aspm_policy has not changed. This is a problem because we
observed that after resume from S3, BIOS has reenabled L1SS. So we can boot with
powersave (which disables L1SS) but resume with L1SS enabled and policy still
set to powersave. This is a preexisting bug. I've observed this behavior on
Thomas's system and with mainline on our desktop systems. This is the reason for
patch [2]. It will force ASPM to be configured in
pcie_aspm_powersave_config_link() even if the policy is the same. It works on my
system. I'm hoping that it will work on his system to resume successfully with
the correct policy enabled.

[1] https://bugzilla.kernel.org/show_bug.cgi?id=216877#c33
[2] https://bugzilla.kernel.org/attachment.cgi?id=305395&action=diff

David

Re: My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

[Bjorn Helgaas]

[+cc Matthew, author of 41cd766b0659 ("PCI: Don't enable aspm before drivers have had a chance to veto it")]

On Thu, Nov 16, 2023 at 12:10:02PM -0800, David E. Box wrote:
> On Tue, 2023-11-07 at 13:15 +0200, Mika Westerberg wrote:
> > On Mon, Nov 06, 2023 at 12:11:07PM -0600, Bjorn Helgaas wrote:
> > > On Sat, Nov 04, 2023 at 10:13:24AM -0700, Kenneth R. Crudup wrote:
> > > > 
> > > > I have a Dell XPS-9320 with an Alderlake chipset, and the NVMe
> > > > behind a VMD device:
> > > > 
> > > > ----
> > > > [    0.127342] smpboot: CPU0: 12th Gen Intel(R) Core(TM) i7-1280P (family:
> > > > 0x6, model: 0x9a, stepping: 0x3)
> > > > ----
> > > > 0000:00:0e.0 0104: 8086:467f
> > > >         Subsystem: 1028:0af3
> > > >         Flags: bus master, fast devsel, latency 0, IOMMU group 9
> > > >         Memory at 603c000000 (64-bit, non-prefetchable) [size=32M]
> > > >         Memory at 72000000 (32-bit, non-prefetchable) [size=32M]
> > > > a7152be79b6        Memory at 6040100000 (64-bit, non-prefetchable)
> > > > [size=1M]
> > > >         Capabilities: <access denied>
> > > >         Kernel driver in use: vmd
> > > > ----
> > > > 
> > > > The only release kernel that was able to get this laptop to
> > > > fully get into low-power (unfortunately only s0ix) was the
> > > > Ubuntu-6.2.0- ... series from Ubuntu (remote
> > > > git://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/lunar).
> > > > 
> > > > I'd bisected it to the following commits (in this order):
> > > > 
> > > > 4ff116d0d5fd PCI/ASPM: Save L1 PM Substates Capability for suspend/resume
> > > > 5e85eba6f50d PCI/ASPM: Refactor L1 PM Substates Control Register
> > > > programming
> > > > 1a0102a08f20 UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under VMD
> > > > domain
> > > > 47c7bfd31514 UBUNTU: SAUCE: PCI/ASPM: Enable LTR for endpoints behind VMD
> > > > 154d48da2c57 UBUNTU: SAUCE: vmd: fixup bridge ASPM by driver name instead
> > > 
> > > Thanks for these.  You don't happen to have URLs for those Ubuntu
> > > commits, do you?  E.g., https://git.kernel.org/linus/4ff116d0d5fd
> > > (which was reverted by a7152be79b62 ("Revert "PCI/ASPM: Save L1 PM
> > > Substates Capability for suspend/resume"")).
> > > 
> > > > Without the patches I never see Pkg%PC8 or higher(? lower?),
> > > > nor i915 states DC5/6, all necssary for SYS%LPI/CPU%LPI. I've
> > > > attached a little script I use alongside turbostat for
> > > > verifying low-power operation (and also for seeing what
> > > > chipset subsystem may be preventing it).
> > > > 
> > > > The first two are in Linus' trees, but were reverted
> > > > (4ff116d0d5fd in a7152be79b6, 5e85eba6f50d in ff209ecc376a).
> > > > The last three come from Ubuntu's Linux trees (see remote spec
> > > > above). The first two remain reverted in the Ubuntu trees, but
> > > > if I put them back, I get increased power savings during
> > > > suspend/resume cycles.
> > > > 
> > > > Considering the power draw is really significant without these
> > > > patches (10s of %s per hour) and I'd think Dell would have
> > > > sold some decent number of these laptops, I'd been patiently
> > > > waiting for these patches, or some variant to show up in the
> > > > stable trees, but so far I'm up to the 6.6 stable kernel and
> > > > still having to manually cherry-pick these, so I thought maybe
> > > > I could bring this to the PM maintainers' attention so at
> > > > least start a discussion about this issue.
> > > 
> > > Thank you very much for raising this again.  We really need to make
> > > some progress, and Mika recently posted a patch to add the
> > > 4ff116d0d5fd functionality again:
> > > https://lore.kernel.org/r/20231002070044.2299644-1-mika.westerberg@linux.intel.com
> > > 
> > > The big problem is that it works on *most* systems, but it still
> > > seems to break a few.  So Mika's current patch relies on a
> > > denylist of systems where we *don't* restore the substates.
> > 
> > According to latest reports it is just that one system where this
> > is still an issue. The latest patch works in Asus UX305FA even if
> > it is not in the denylist. That would leave that one system only
> > to the denylist, at least the ones we are aware about.
> 
> I've been working with Thomas, whose system is the last known to
> have problems with Mika's patch. It turns out that his config sets
> aspm_policy to 'powersave'.  If he sets it to any other policy,
> Mika's patch works [1]. It's possible that others may see the same
> issue if they use 'powersave' as well.
> 
> The theory right now is that enabling L1SS in pci_restore_state() is
> too early.  

I'd really like to figure out what "too early" means.  We can make it
later by enabling L1SS somewhere else, but unless we know exactly what
needs to happen first, we're likely to break it again.  And if we know
what's required, we can probably figure out a cleaner way to restore
it.

> During boot, if ASPM policy is 'powersave' or
> 'powersupersave', ASPM enabling is deferred. The comment in
> pcie_aspm_init_link_state() that skips it state that:
> 
>         /*
>          * At this stage drivers haven't had an opportunity to change the
>          * link policy setting. Enabling ASPM on broken hardware can cripple
>          * it even before the driver has had a chance to disable ASPM, so
>          * default to a safe level right now. If we're enabling ASPM beyond
>          * the BIOS's expectation, we'll do so once pci_enable_device() is
>          * called.

This is from 41cd766b0659 ("PCI: Don't enable aspm before drivers have
had a chance to veto it") [3].  I assume the idea is that driver probe
methods can use pci_disable_link_state() to veto certain link states.

I think this would be better as a quirk instead of a driver probe
method because I don't think ASPM really has anything to do with the
driver probe.  We do most ASPM configuration at enumeration (before
driver probe), so now we have this exception that we delay it until
probe time if the policy is POLICY_POWERSAVE or
POLICY_POWER_SUPERSAVE.

There are only about a dozen callers of pci_disable_link_state(), so
it doesn't seem impossible to change them to use quirks instead, e.g.,
quirk_disable_aspm_l0s() and quirk_disable_aspm_l0s_l1().

> While pci_enable_device() is called by the PCI core before
> pci_restore_state() on resume, it is called again later by the nvme
> driver in nvme_pci_enable().  This stage seems the intended
> intercept mentioned in the comment. This ends up calling
> pcie_aspm_powersave_config_link() to configure ASPM at that time.
> During boot we see ASPM enabling is indeed happening for powersave
> during nvme_pci_enable(). With the save/restore patch however it is
> being restored before nvme_pci_enable(). I've asked Thomas not to
> apply Mika's patch, but instead use a different patch [2] that waits
> until pcie_aspm_powersave_config_link() is called to configure ASPM.
> The need for this is mentioned below. Hopefully it will fix the hang
> observed on his system.
> 
> Whether that patch works for him, we can address his problem with
> the current L1SS save/restore patch by removing the current denylist
> and instead only do the save/restore if ASPM policy is 'default'
> which doesn't hang his system. This makes sense since it's only the
> BIOS config that we care to preserve since it can be lost during
> suspend, particularly during s2idle. All other policies are OS
> controlled if allowed. Instead of save/restore for those we can let
> it be configured later when pcie_aspm_powersave_config_link() is
> called.
> 
> The only issue with this is that pcie_aspm_powersave_config_link()
> will not configure ASPM if aspm_policy has not changed. This is a
> problem because we observed that after resume from S3, BIOS has
> reenabled L1SS. So we can boot with powersave (which disables L1SS)
> but resume with L1SS enabled and policy still set to powersave. This
> is a preexisting bug. I've observed this behavior on Thomas's system
> and with mainline on our desktop systems. This is the reason for
> patch [2]. It will force ASPM to be configured in
> pcie_aspm_powersave_config_link() even if the policy is the same. It
> works on my system. I'm hoping that it will work on his system to
> resume successfully with the correct policy enabled.
> 
> [1] https://bugzilla.kernel.org/show_bug.cgi?id=216877#c33
> [2] https://bugzilla.kernel.org/attachment.cgi?id=305395&action=diff

[3] https://git.kernel.org/linus/41cd766b0659

Re: My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

[Matthew Garrett]

On Thu, Nov 16, 2023 at 05:18:12PM -0600, Bjorn Helgaas wrote:

> I think this would be better as a quirk instead of a driver probe
> method because I don't think ASPM really has anything to do with the
> driver probe.  We do most ASPM configuration at enumeration (before
> driver probe), so now we have this exception that we delay it until
> probe time if the policy is POLICY_POWERSAVE or
> POLICY_POWER_SUPERSAVE.

I think doing this as a quirk would probably work fine, but from an 
aesthetic point of view it feels awkward - this is knowledge that the 
drivers have, and so fundamentally placing that knowledge in the core 
PCI code feels like the wrong place to put it.

Re: My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

[David E. Box]

On Thu, 2023-11-16 at 17:18 -0600, Bjorn Helgaas wrote:
> [+cc Matthew, author of 41cd766b0659 ("PCI: Don't enable aspm before drivers
> have had a chance to veto it")]
> 
> On Thu, Nov 16, 2023 at 12:10:02PM -0800, David E. Box wrote:
> > On Tue, 2023-11-07 at 13:15 +0200, Mika Westerberg wrote:
> > > On Mon, Nov 06, 2023 at 12:11:07PM -0600, Bjorn Helgaas wrote:
> > > > On Sat, Nov 04, 2023 at 10:13:24AM -0700, Kenneth R. Crudup wrote:
> > > > > 
> > > > > I have a Dell XPS-9320 with an Alderlake chipset, and the NVMe
> > > > > behind a VMD device:
> > > > > 
> > > > > ----
> > > > > [    0.127342] smpboot: CPU0: 12th Gen Intel(R) Core(TM) i7-1280P
> > > > > (family:
> > > > > 0x6, model: 0x9a, stepping: 0x3)
> > > > > ----
> > > > > 0000:00:0e.0 0104: 8086:467f
> > > > >         Subsystem: 1028:0af3
> > > > >         Flags: bus master, fast devsel, latency 0, IOMMU group 9
> > > > >         Memory at 603c000000 (64-bit, non-prefetchable) [size=32M]
> > > > >         Memory at 72000000 (32-bit, non-prefetchable) [size=32M]
> > > > > a7152be79b6        Memory at 6040100000 (64-bit, non-prefetchable)
> > > > > [size=1M]
> > > > >         Capabilities: <access denied>
> > > > >         Kernel driver in use: vmd
> > > > > ----
> > > > > 
> > > > > The only release kernel that was able to get this laptop to
> > > > > fully get into low-power (unfortunately only s0ix) was the
> > > > > Ubuntu-6.2.0- ... series from Ubuntu (remote
> > > > > git://git.launchpad.net/~ubuntu-
> > > > > kernel/ubuntu/+source/linux/+git/lunar).
> > > > > 
> > > > > I'd bisected it to the following commits (in this order):
> > > > > 
> > > > > 4ff116d0d5fd PCI/ASPM: Save L1 PM Substates Capability for
> > > > > suspend/resume
> > > > > 5e85eba6f50d PCI/ASPM: Refactor L1 PM Substates Control Register
> > > > > programming
> > > > > 1a0102a08f20 UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under VMD
> > > > > domain
> > > > > 47c7bfd31514 UBUNTU: SAUCE: PCI/ASPM: Enable LTR for endpoints behind
> > > > > VMD
> > > > > 154d48da2c57 UBUNTU: SAUCE: vmd: fixup bridge ASPM by driver name
> > > > > instead
> > > > 
> > > > Thanks for these.  You don't happen to have URLs for those Ubuntu
> > > > commits, do you?  E.g., https://git.kernel.org/linus/4ff116d0d5fd
> > > > (which was reverted by a7152be79b62 ("Revert "PCI/ASPM: Save L1 PM
> > > > Substates Capability for suspend/resume"")).
> > > > 
> > > > > Without the patches I never see Pkg%PC8 or higher(? lower?),
> > > > > nor i915 states DC5/6, all necssary for SYS%LPI/CPU%LPI. I've
> > > > > attached a little script I use alongside turbostat for
> > > > > verifying low-power operation (and also for seeing what
> > > > > chipset subsystem may be preventing it).
> > > > > 
> > > > > The first two are in Linus' trees, but were reverted
> > > > > (4ff116d0d5fd in a7152be79b6, 5e85eba6f50d in ff209ecc376a).
> > > > > The last three come from Ubuntu's Linux trees (see remote spec
> > > > > above). The first two remain reverted in the Ubuntu trees, but
> > > > > if I put them back, I get increased power savings during
> > > > > suspend/resume cycles.
> > > > > 
> > > > > Considering the power draw is really significant without these
> > > > > patches (10s of %s per hour) and I'd think Dell would have
> > > > > sold some decent number of these laptops, I'd been patiently
> > > > > waiting for these patches, or some variant to show up in the
> > > > > stable trees, but so far I'm up to the 6.6 stable kernel and
> > > > > still having to manually cherry-pick these, so I thought maybe
> > > > > I could bring this to the PM maintainers' attention so at
> > > > > least start a discussion about this issue.
> > > > 
> > > > Thank you very much for raising this again.  We really need to make
> > > > some progress, and Mika recently posted a patch to add the
> > > > 4ff116d0d5fd functionality again:
> > > > https://lore.kernel.org/r/20231002070044.2299644-1-mika.westerberg@linux.intel.com
> > > > 
> > > > The big problem is that it works on *most* systems, but it still
> > > > seems to break a few.  So Mika's current patch relies on a
> > > > denylist of systems where we *don't* restore the substates.
> > > 
> > > According to latest reports it is just that one system where this
> > > is still an issue. The latest patch works in Asus UX305FA even if
> > > it is not in the denylist. That would leave that one system only
> > > to the denylist, at least the ones we are aware about.
> > 
> > I've been working with Thomas, whose system is the last known to
> > have problems with Mika's patch. It turns out that his config sets
> > aspm_policy to 'powersave'.  If he sets it to any other policy,
> > Mika's patch works [1]. It's possible that others may see the same
> > issue if they use 'powersave' as well.
> > 
> > The theory right now is that enabling L1SS in pci_restore_state() is
> > too early.  
> 
> I'd really like to figure out what "too early" means.  We can make it
> later by enabling L1SS somewhere else, but unless we know exactly what
> needs to happen first, we're likely to break it again.  And if we know
> what's required, we can probably figure out a cleaner way to restore
> it.

Still trying to understand this particular failure. The current patch to Thomas
more closely mimics how ASPM is enabled during boot when powersave is set. If it
works we can at least prove that we can get it to work again by using a similar
flow.

David

> 
> > During boot, if ASPM policy is 'powersave' or
> > 'powersupersave', ASPM enabling is deferred. The comment in
> > pcie_aspm_init_link_state() that skips it state that:
> > 
> >         /*
> >          * At this stage drivers haven't had an opportunity to change the
> >          * link policy setting. Enabling ASPM on broken hardware can cripple
> >          * it even before the driver has had a chance to disable ASPM, so
> >          * default to a safe level right now. If we're enabling ASPM beyond
> >          * the BIOS's expectation, we'll do so once pci_enable_device() is
> >          * called.
> 
> This is from 41cd766b0659 ("PCI: Don't enable aspm before drivers have
> had a chance to veto it") [3].  I assume the idea is that driver probe
> methods can use pci_disable_link_state() to veto certain link states.
> 
> I think this would be better as a quirk instead of a driver probe
> method because I don't think ASPM really has anything to do with the
> driver probe.  We do most ASPM configuration at enumeration (before
> driver probe), so now we have this exception that we delay it until
> probe time if the policy is POLICY_POWERSAVE or
> POLICY_POWER_SUPERSAVE.
> 
> There are only about a dozen callers of pci_disable_link_state(), so
> it doesn't seem impossible to change them to use quirks instead, e.g.,
> quirk_disable_aspm_l0s() and quirk_disable_aspm_l0s_l1().
> 
> > While pci_enable_device() is called by the PCI core before
> > pci_restore_state() on resume, it is called again later by the nvme
> > driver in nvme_pci_enable().  This stage seems the intended
> > intercept mentioned in the comment. This ends up calling
> > pcie_aspm_powersave_config_link() to configure ASPM at that time.
> > During boot we see ASPM enabling is indeed happening for powersave
> > during nvme_pci_enable(). With the save/restore patch however it is
> > being restored before nvme_pci_enable(). I've asked Thomas not to
> > apply Mika's patch, but instead use a different patch [2] that waits
> > until pcie_aspm_powersave_config_link() is called to configure ASPM.
> > The need for this is mentioned below. Hopefully it will fix the hang
> > observed on his system.
> > 
> > Whether that patch works for him, we can address his problem with
> > the current L1SS save/restore patch by removing the current denylist
> > and instead only do the save/restore if ASPM policy is 'default'
> > which doesn't hang his system. This makes sense since it's only the
> > BIOS config that we care to preserve since it can be lost during
> > suspend, particularly during s2idle. All other policies are OS
> > controlled if allowed. Instead of save/restore for those we can let
> > it be configured later when pcie_aspm_powersave_config_link() is
> > called.
> > 
> > The only issue with this is that pcie_aspm_powersave_config_link()
> > will not configure ASPM if aspm_policy has not changed. This is a
> > problem because we observed that after resume from S3, BIOS has
> > reenabled L1SS. So we can boot with powersave (which disables L1SS)
> > but resume with L1SS enabled and policy still set to powersave. This
> > is a preexisting bug. I've observed this behavior on Thomas's system
> > and with mainline on our desktop systems. This is the reason for
> > patch [2]. It will force ASPM to be configured in
> > pcie_aspm_powersave_config_link() even if the policy is the same. It
> > works on my system. I'm hoping that it will work on his system to
> > resume successfully with the correct policy enabled.
> > 
> > [1] https://bugzilla.kernel.org/show_bug.cgi?id=216877#c33
> > [2] https://bugzilla.kernel.org/attachment.cgi?id=305395&action=diff
> 
> [3] https://git.kernel.org/linus/41cd766b0659

Re: My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

[David E. Box]

On Fri, 2023-11-17 at 16:21 -0800, David E. Box wrote:
> On Thu, 2023-11-16 at 17:18 -0600, Bjorn Helgaas wrote:
> > [+cc Matthew, author of 41cd766b0659 ("PCI: Don't enable aspm before drivers
> > have had a chance to veto it")]
> > 
> > On Thu, Nov 16, 2023 at 12:10:02PM -0800, David E. Box wrote:
> > > On Tue, 2023-11-07 at 13:15 +0200, Mika Westerberg wrote:
> > > > On Mon, Nov 06, 2023 at 12:11:07PM -0600, Bjorn Helgaas wrote:
> > > > > On Sat, Nov 04, 2023 at 10:13:24AM -0700, Kenneth R. Crudup wrote:
> > > > > > 
> > > > > > I have a Dell XPS-9320 with an Alderlake chipset, and the NVMe
> > > > > > behind a VMD device:
> > > > > > 
> > > > > > ----
> > > > > > [    0.127342] smpboot: CPU0: 12th Gen Intel(R) Core(TM) i7-1280P
> > > > > > (family:
> > > > > > 0x6, model: 0x9a, stepping: 0x3)
> > > > > > ----
> > > > > > 0000:00:0e.0 0104: 8086:467f
> > > > > >         Subsystem: 1028:0af3
> > > > > >         Flags: bus master, fast devsel, latency 0, IOMMU group 9
> > > > > >         Memory at 603c000000 (64-bit, non-prefetchable) [size=32M]
> > > > > >         Memory at 72000000 (32-bit, non-prefetchable) [size=32M]
> > > > > > a7152be79b6        Memory at 6040100000 (64-bit, non-prefetchable)
> > > > > > [size=1M]
> > > > > >         Capabilities: <access denied>
> > > > > >         Kernel driver in use: vmd
> > > > > > ----
> > > > > > 
> > > > > > The only release kernel that was able to get this laptop to
> > > > > > fully get into low-power (unfortunately only s0ix) was the
> > > > > > Ubuntu-6.2.0- ... series from Ubuntu (remote
> > > > > > git://git.launchpad.net/~ubuntu-
> > > > > > kernel/ubuntu/+source/linux/+git/lunar).
> > > > > > 
> > > > > > I'd bisected it to the following commits (in this order):
> > > > > > 
> > > > > > 4ff116d0d5fd PCI/ASPM: Save L1 PM Substates Capability for
> > > > > > suspend/resume
> > > > > > 5e85eba6f50d PCI/ASPM: Refactor L1 PM Substates Control Register
> > > > > > programming
> > > > > > 1a0102a08f20 UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under
> > > > > > VMD
> > > > > > domain
> > > > > > 47c7bfd31514 UBUNTU: SAUCE: PCI/ASPM: Enable LTR for endpoints
> > > > > > behind
> > > > > > VMD
> > > > > > 154d48da2c57 UBUNTU: SAUCE: vmd: fixup bridge ASPM by driver name
> > > > > > instead
> > > > > 
> > > > > Thanks for these.  You don't happen to have URLs for those Ubuntu
> > > > > commits, do you?  E.g., https://git.kernel.org/linus/4ff116d0d5fd
> > > > > (which was reverted by a7152be79b62 ("Revert "PCI/ASPM: Save L1 PM
> > > > > Substates Capability for suspend/resume"")).
> > > > > 
> > > > > > Without the patches I never see Pkg%PC8 or higher(? lower?),
> > > > > > nor i915 states DC5/6, all necssary for SYS%LPI/CPU%LPI. I've
> > > > > > attached a little script I use alongside turbostat for
> > > > > > verifying low-power operation (and also for seeing what
> > > > > > chipset subsystem may be preventing it).
> > > > > > 
> > > > > > The first two are in Linus' trees, but were reverted
> > > > > > (4ff116d0d5fd in a7152be79b6, 5e85eba6f50d in ff209ecc376a).
> > > > > > The last three come from Ubuntu's Linux trees (see remote spec
> > > > > > above). The first two remain reverted in the Ubuntu trees, but
> > > > > > if I put them back, I get increased power savings during
> > > > > > suspend/resume cycles.
> > > > > > 
> > > > > > Considering the power draw is really significant without these
> > > > > > patches (10s of %s per hour) and I'd think Dell would have
> > > > > > sold some decent number of these laptops, I'd been patiently
> > > > > > waiting for these patches, or some variant to show up in the
> > > > > > stable trees, but so far I'm up to the 6.6 stable kernel and
> > > > > > still having to manually cherry-pick these, so I thought maybe
> > > > > > I could bring this to the PM maintainers' attention so at
> > > > > > least start a discussion about this issue.
> > > > > 
> > > > > Thank you very much for raising this again.  We really need to make
> > > > > some progress, and Mika recently posted a patch to add the
> > > > > 4ff116d0d5fd functionality again:
> > > > > https://lore.kernel.org/r/20231002070044.2299644-1-mika.westerberg@linux.intel.com
> > > > > 
> > > > > The big problem is that it works on *most* systems, but it still
> > > > > seems to break a few.  So Mika's current patch relies on a
> > > > > denylist of systems where we *don't* restore the substates.
> > > > 
> > > > According to latest reports it is just that one system where this
> > > > is still an issue. The latest patch works in Asus UX305FA even if
> > > > it is not in the denylist. That would leave that one system only
> > > > to the denylist, at least the ones we are aware about.
> > > 
> > > I've been working with Thomas, whose system is the last known to
> > > have problems with Mika's patch. It turns out that his config sets
> > > aspm_policy to 'powersave'.  If he sets it to any other policy,
> > > Mika's patch works [1]. It's possible that others may see the same
> > > issue if they use 'powersave' as well.
> > > 
> > > The theory right now is that enabling L1SS in pci_restore_state() is
> > > too early.  
> > 
> > I'd really like to figure out what "too early" means.  We can make it
> > later by enabling L1SS somewhere else, but unless we know exactly what
> > needs to happen first, we're likely to break it again.  And if we know
> > what's required, we can probably figure out a cleaner way to restore
> > it.
> 
> Still trying to understand this particular failure. The current patch to
> Thomas
> more closely mimics how ASPM is enabled during boot when powersave is set. If
> it
> works we can at least prove that we can get it to work again by using a
> similar
> flow.


With some free time I was able to find a system in our lab that reproduces the
same failure reported on the last problem report from Thomas. That is, with
powersave selected, the nvme fails to come up after resume from S3 with this
patch without a quirk. It's actually obvious when you can see the flow. We
observed that on S3 resume, BIOS has enabled L1.2 (likely back to preboot
setting). Restoring powersave will therefore disable L1.2. Per spec, L1.2 must
be disabled on the downstream first. But pci_restore_state() gets called on
upstream devices first. Indeed, on my system, clearing the L1.2 state on the
root port makes the nvme device inaccessible by the time
pci_aspm_restore_state() is called for it. I've modified the patch to defer L1SS
restore until the downstream component so they can be done together. The patch
clears L1.2 on the child first before the parent, restores both configs and then
reenables them in reverse on the parent then the child. This works on my system.
I've posted the patch as a V5 and on the bugzilla and appreciate if anyone here
can test.

https://lore.kernel.org/linux-pci/20231221011250.191599-1-david.e.box@linux.intel.com/

David

Re: My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

[Bjorn Helgaas]

On Wed, Dec 20, 2023 at 05:19:34PM -0800, David E. Box wrote:
> On Fri, 2023-11-17 at 16:21 -0800, David E. Box wrote:
> > On Thu, 2023-11-16 at 17:18 -0600, Bjorn Helgaas wrote:
> > > [+cc Matthew, author of 41cd766b0659 ("PCI: Don't enable aspm before drivers
> > > have had a chance to veto it")]
> > > 
> > > On Thu, Nov 16, 2023 at 12:10:02PM -0800, David E. Box wrote:
> > > > On Tue, 2023-11-07 at 13:15 +0200, Mika Westerberg wrote:
> > > > > On Mon, Nov 06, 2023 at 12:11:07PM -0600, Bjorn Helgaas wrote:
> > > > > > On Sat, Nov 04, 2023 at 10:13:24AM -0700, Kenneth R. Crudup wrote:
> > > > > > > 
> > > > > > > I have a Dell XPS-9320 with an Alderlake chipset, and the NVMe
> > > > > > > behind a VMD device:
> > > > > > > 
> > > > > > > ----
> > > > > > > [    0.127342] smpboot: CPU0: 12th Gen Intel(R) Core(TM) i7-1280P
> > > > > > > (family:
> > > > > > > 0x6, model: 0x9a, stepping: 0x3)
> > > > > > > ----
> > > > > > > 0000:00:0e.0 0104: 8086:467f
> > > > > > >         Subsystem: 1028:0af3
> > > > > > >         Flags: bus master, fast devsel, latency 0, IOMMU group 9
> > > > > > >         Memory at 603c000000 (64-bit, non-prefetchable) [size=32M]
> > > > > > >         Memory at 72000000 (32-bit, non-prefetchable) [size=32M]
> > > > > > > a7152be79b6        Memory at 6040100000 (64-bit, non-prefetchable)
> > > > > > > [size=1M]
> > > > > > >         Capabilities: <access denied>
> > > > > > >         Kernel driver in use: vmd
> > > > > > > ----
> > > > > > > 
> > > > > > > The only release kernel that was able to get this laptop to
> > > > > > > fully get into low-power (unfortunately only s0ix) was the
> > > > > > > Ubuntu-6.2.0- ... series from Ubuntu (remote
> > > > > > > git://git.launchpad.net/~ubuntu-
> > > > > > > kernel/ubuntu/+source/linux/+git/lunar).
> > > > > > > 
> > > > > > > I'd bisected it to the following commits (in this order):
> > > > > > > 
> > > > > > > 4ff116d0d5fd PCI/ASPM: Save L1 PM Substates Capability for
> > > > > > > suspend/resume
> > > > > > > 5e85eba6f50d PCI/ASPM: Refactor L1 PM Substates Control Register
> > > > > > > programming
> > > > > > > 1a0102a08f20 UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under
> > > > > > > VMD
> > > > > > > domain
> > > > > > > 47c7bfd31514 UBUNTU: SAUCE: PCI/ASPM: Enable LTR for endpoints
> > > > > > > behind
> > > > > > > VMD
> > > > > > > 154d48da2c57 UBUNTU: SAUCE: vmd: fixup bridge ASPM by driver name
> > > > > > > instead
> > > > > > 
> > > > > > Thanks for these.  You don't happen to have URLs for those Ubuntu
> > > > > > commits, do you?  E.g., https://git.kernel.org/linus/4ff116d0d5fd
> > > > > > (which was reverted by a7152be79b62 ("Revert "PCI/ASPM: Save L1 PM
> > > > > > Substates Capability for suspend/resume"")).
> > > > > > 
> > > > > > > Without the patches I never see Pkg%PC8 or higher(? lower?),
> > > > > > > nor i915 states DC5/6, all necssary for SYS%LPI/CPU%LPI. I've
> > > > > > > attached a little script I use alongside turbostat for
> > > > > > > verifying low-power operation (and also for seeing what
> > > > > > > chipset subsystem may be preventing it).
> > > > > > > 
> > > > > > > The first two are in Linus' trees, but were reverted
> > > > > > > (4ff116d0d5fd in a7152be79b6, 5e85eba6f50d in ff209ecc376a).
> > > > > > > The last three come from Ubuntu's Linux trees (see remote spec
> > > > > > > above). The first two remain reverted in the Ubuntu trees, but
> > > > > > > if I put them back, I get increased power savings during
> > > > > > > suspend/resume cycles.
> > > > > > > 
> > > > > > > Considering the power draw is really significant without these
> > > > > > > patches (10s of %s per hour) and I'd think Dell would have
> > > > > > > sold some decent number of these laptops, I'd been patiently
> > > > > > > waiting for these patches, or some variant to show up in the
> > > > > > > stable trees, but so far I'm up to the 6.6 stable kernel and
> > > > > > > still having to manually cherry-pick these, so I thought maybe
> > > > > > > I could bring this to the PM maintainers' attention so at
> > > > > > > least start a discussion about this issue.
> > > > > > 
> > > > > > Thank you very much for raising this again.  We really need to make
> > > > > > some progress, and Mika recently posted a patch to add the
> > > > > > 4ff116d0d5fd functionality again:
> > > > > > https://lore.kernel.org/r/20231002070044.2299644-1-mika.westerberg@linux.intel.com
> > > > > > 
> > > > > > The big problem is that it works on *most* systems, but it still
> > > > > > seems to break a few.  So Mika's current patch relies on a
> > > > > > denylist of systems where we *don't* restore the substates.
> > > > > 
> > > > > According to latest reports it is just that one system where this
> > > > > is still an issue. The latest patch works in Asus UX305FA even if
> > > > > it is not in the denylist. That would leave that one system only
> > > > > to the denylist, at least the ones we are aware about.
> > > > 
> > > > I've been working with Thomas, whose system is the last known to
> > > > have problems with Mika's patch. It turns out that his config sets
> > > > aspm_policy to 'powersave'.  If he sets it to any other policy,
> > > > Mika's patch works [1]. It's possible that others may see the same
> > > > issue if they use 'powersave' as well.
> > > > 
> > > > The theory right now is that enabling L1SS in pci_restore_state() is
> > > > too early.  
> > > 
> > > I'd really like to figure out what "too early" means.  We can
> > > make it later by enabling L1SS somewhere else, but unless we
> > > know exactly what needs to happen first, we're likely to break
> > > it again.  And if we know what's required, we can probably
> > > figure out a cleaner way to restore it.
> > 
> > Still trying to understand this particular failure. The current
> > patch to Thomas more closely mimics how ASPM is enabled during
> > boot when powersave is set. If it works we can at least prove that
> > we can get it to work again by using a similar flow.
> 
> With some free time I was able to find a system in our lab that
> reproduces the same failure reported on the last problem report from
> Thomas. That is, with powersave selected, the nvme fails to come up
> after resume from S3 with this patch without a quirk. It's actually
> obvious when you can see the flow. We observed that on S3 resume,
> BIOS has enabled L1.2 (likely back to preboot setting). Restoring
> powersave will therefore disable L1.2. Per spec, L1.2 must be
> disabled on the downstream first. But pci_restore_state() gets
> called on upstream devices first. Indeed, on my system, clearing the
> L1.2 state on the root port makes the nvme device inaccessible by
> the time pci_aspm_restore_state() is called for it. I've modified
> the patch to defer L1SS restore until the downstream component so
> they can be done together. The patch clears L1.2 on the child first
> before the parent, restores both configs and then reenables them in
> reverse on the parent then the child. This works on my system.  I've
> posted the patch as a V5 and on the bugzilla and appreciate if
> anyone here can test.

This is FANTASTIC!  Thank you so much for getting to the bottom of
this!

Bjorn

Re: My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

[Kenneth R. Crudup]

[-- Attachment #1: Type: text/plain, Size: 1168 bytes --]


On Sat, 4 Nov 2023, Kenneth R. Crudup wrote:

> The only release kernel that was able to get this laptop to fully get into
> low-power (unfortunately only s0ix) was the Ubuntu-6.2.0- ... series from Ubuntu

> I'd bisected it to the following commits:
> 4ff116d0d5fd PCI/ASPM: Save L1 PM Substates Capability for suspend/resume
> 5e85eba6f50d PCI/ASPM: Refactor L1 PM Substates Control Register programming
> 1a0102a08f20 UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under VMD domain
> 47c7bfd31514 UBUNTU: SAUCE: PCI/ASPM: Enable LTR for endpoints behind VMD
> 154d48da2c57 UBUNTU: SAUCE: vmd: fixup bridge ASPM by driver name instead

Since (for what I'm sure is a good reason ... I hope :) ) this has yet to make
it into mainline, here's the set of commits refactored for v6.8; maybe someone
scanning the archives for a solution to their Dell draining too much power can
use them.

But is there anything I can do to help these go in? I saw that "Refactor L1
PM Substates Control Register programming" is still reverted, is that still
an issue on the machine it affected?

	-Kenny

-- 
Kenneth R. Crudup / Sr. SW Engineer, Scott County Consulting, Orange County CA

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-diff; name=0005-UBUNTU-SAUCE-vmd-fixup-bridge-ASPM-by-driver-name-in.patch, Size: 3069 bytes --]

From 646a1445e59ac1310ad71ffd3663160843e66e21 Mon Sep 17 00:00:00 2001
From: You-Sheng Yang <vicamo.yang@canonical.com>
Date: Mon, 11 Apr 2022 17:24:34 +0800
Subject: [PATCH 5/5] UBUNTU: SAUCE: vmd: fixup bridge ASPM by driver name
 instead

BugLink: https://bugs.launchpad.net/bugs/1942160

Additional VMD bridge IDs needed for new Alder Lake platforms, but
actually there is no a complete list for them. Here we match bridge
devices if they're directly attached to a VMD controller instead.

Signed-off-by: You-Sheng Yang <vicamo.yang@canonical.com>
Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
(cherry picked from commit 154d48da2c57514e4b5dadc7b8c70a4edb550981)
---
 drivers/pci/quirks.c | 32 +++++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index b00b996c7485..1f03d77907f2 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -6251,23 +6251,36 @@ static void pci_fixup_d3cold_delay_1sec(struct pci_dev *pdev)
 }
 DECLARE_PCI_FIXUP_FINAL(0x5555, 0x0004, pci_fixup_d3cold_delay_1sec);
 /*
- * Device [8086:9a09], [8086:a0b0] and [8086:a0bc]
  * BIOS may not be able to access config space of devices under VMD domain, so
  * it relies on software to enable ASPM for links under VMD.
  */
-static const struct pci_device_id vmd_bridge_tbl[] = {
-	{ PCI_VDEVICE(INTEL, 0x9a09) },
-	{ PCI_VDEVICE(INTEL, 0xa0b0) },
-	{ PCI_VDEVICE(INTEL, 0xa0bc) },
-	{ }
-};
+static bool pci_fixup_is_vmd_bridge(struct pci_dev *pdev)
+{
+	struct pci_bus *bus = pdev->bus;
+	struct device *dev;
+	struct pci_driver *pdrv;
+
+	if (!pci_is_root_bus(bus))
+		return false;
+
+	dev = bus->bridge->parent;
+	if (dev == NULL)
+		return false;
+
+	pdrv = pci_dev_driver(to_pci_dev(dev));
+	if (pdrv == NULL || strcmp("vmd", pdrv->name))
+		return false;
+
+	return true;
+}
 
 static void pci_fixup_enable_aspm(struct pci_dev *pdev)
 {
-	if (!pci_match_id(vmd_bridge_tbl, pdev))
+	if (!pci_fixup_is_vmd_bridge(pdev))
 		return;
 
 	pdev->dev_flags |= PCI_DEV_FLAGS_ENABLE_ASPM;
+	pci_info(pdev, "enable ASPM for pci bridge behind vmd");
 }
 DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_VENDOR_ID_INTEL, PCI_ANY_ID,
 			       PCI_CLASS_BRIDGE_PCI, 8, pci_fixup_enable_aspm);
@@ -6282,7 +6295,7 @@ static void pci_fixup_enable_vmd_nvme_ltr(struct pci_dev *pdev)
 	if (!parent)
 		return;
 
-	if (!pci_match_id(vmd_bridge_tbl, parent))
+	if (!pci_fixup_is_vmd_bridge(parent))
 		return;
 
 	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_LTR);
@@ -6300,6 +6313,7 @@ static void pci_fixup_enable_vmd_nvme_ltr(struct pci_dev *pdev)
 	/* 3145728ns, i.e. 0x300000ns */
 	pci_write_config_word(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, 0x1003);
 	pci_write_config_word(pdev, pos + PCI_LTR_MAX_NOSNOOP_LAT, 0x1003);
+	pci_info(pdev, "enable LTR for nvme behind vmd");
 }
 DECLARE_PCI_FIXUP_CLASS_EARLY(PCI_ANY_ID, PCI_ANY_ID,
 			      PCI_CLASS_STORAGE_EXPRESS, 0, pci_fixup_enable_vmd_nvme_ltr);
-- 
2.34.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: Type: text/x-diff; name=0004-UBUNTU-SAUCE-PCI-ASPM-Enable-LTR-for-endpoints-behin.patch, Size: 2935 bytes --]

From a3e6988bc1141c3d1f085e274d68f5d72de6a6b1 Mon Sep 17 00:00:00 2001
From: Kai-Heng Feng <kai.heng.feng@canonical.com>
Date: Mon, 11 Apr 2022 17:24:33 +0800
Subject: [PATCH 4/5] UBUNTU: SAUCE: PCI/ASPM: Enable LTR for endpoints behind
 VMD

BugLink: https://bugs.launchpad.net/bugs/1942160

In addition to ASPM, LTR also needs to be programmed with a reasonable
value to let PCIe link reaches L1.2.

For now, program a hardcoded value that is used under Windows.

While at it, consolidate ASPM and LTR enabling logic to share a same pci
device table.

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
(cherry picked from commit 47c7bfd31514e7b54a1f830f7707297aebbb8679)
---
 drivers/pci/quirks.c | 48 +++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 45 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index a50174619c20..b00b996c7485 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -6251,13 +6251,55 @@ static void pci_fixup_d3cold_delay_1sec(struct pci_dev *pdev)
 }
 DECLARE_PCI_FIXUP_FINAL(0x5555, 0x0004, pci_fixup_d3cold_delay_1sec);
 /*
- * Device [8086:9a09]
+ * Device [8086:9a09], [8086:a0b0] and [8086:a0bc]
  * BIOS may not be able to access config space of devices under VMD domain, so
  * it relies on software to enable ASPM for links under VMD.
  */
+static const struct pci_device_id vmd_bridge_tbl[] = {
+	{ PCI_VDEVICE(INTEL, 0x9a09) },
+	{ PCI_VDEVICE(INTEL, 0xa0b0) },
+	{ PCI_VDEVICE(INTEL, 0xa0bc) },
+	{ }
+};
+
 static void pci_fixup_enable_aspm(struct pci_dev *pdev)
 {
+	if (!pci_match_id(vmd_bridge_tbl, pdev))
+		return;
+
 	pdev->dev_flags |= PCI_DEV_FLAGS_ENABLE_ASPM;
 }
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x9a09, pci_fixup_enable_aspm);
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0xa0b0, pci_fixup_enable_aspm);
+DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_VENDOR_ID_INTEL, PCI_ANY_ID,
+			       PCI_CLASS_BRIDGE_PCI, 8, pci_fixup_enable_aspm);
+
+static void pci_fixup_enable_vmd_nvme_ltr(struct pci_dev *pdev)
+{
+	struct pci_dev *parent;
+	int pos;
+	u16 val;
+
+	parent = pci_upstream_bridge(pdev);
+	if (!parent)
+		return;
+
+	if (!pci_match_id(vmd_bridge_tbl, parent))
+		return;
+
+	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_LTR);
+	if (!pos)
+		return;
+
+	pci_read_config_word(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, &val);
+	if (val)
+		return;
+
+	pci_read_config_word(pdev, pos + PCI_LTR_MAX_NOSNOOP_LAT, &val);
+	if (val)
+		return;
+
+	/* 3145728ns, i.e. 0x300000ns */
+	pci_write_config_word(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, 0x1003);
+	pci_write_config_word(pdev, pos + PCI_LTR_MAX_NOSNOOP_LAT, 0x1003);
+}
+DECLARE_PCI_FIXUP_CLASS_EARLY(PCI_ANY_ID, PCI_ANY_ID,
+			      PCI_CLASS_STORAGE_EXPRESS, 0, pci_fixup_enable_vmd_nvme_ltr);
-- 
2.34.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: Type: text/x-diff; name=0003-UBUNTU-SAUCE-PCI-ASPM-Enable-ASPM-for-links-under-VM.patch, Size: 3199 bytes --]

From 0fd1d04a09320bc0857b3ccd26a959b7127c151c Mon Sep 17 00:00:00 2001
From: Kai-Heng Feng <kai.heng.feng@canonical.com>
Date: Mon, 11 Apr 2022 17:24:32 +0800
Subject: [PATCH 3/5] UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under VMD
 domain

BugLink: https://bugs.launchpad.net/bugs/1942160

New Intel laptops with VMD cannot reach deeper power saving state,
renders very short battery time.

As BIOS may not be able to program the config space for devices under
VMD domain, ASPM needs to be programmed manually by software. This is
also the case under Windows.

The VMD controller itself is a root complex integrated endpoint that
doesn't have ASPM capability, so we can't propagate the ASPM settings to
devices under it. Hence, simply apply ASPM_STATE_ALL to the links under
VMD domain, unsupported states will be cleared out anyway.

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
(cherry picked from commit 1a0102a08f206149d9abd56c2b28877c878b5526)
---
 drivers/pci/pcie/aspm.c |  3 ++-
 drivers/pci/quirks.c    | 11 +++++++++++
 include/linux/pci.h     |  2 ++
 3 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 840da5be765e..7afab9959be6 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -645,7 +645,8 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	aspm_l1ss_init(link);
 
 	/* Save default state */
-	link->aspm_default = link->aspm_enabled;
+	link->aspm_default = parent->dev_flags & PCI_DEV_FLAGS_ENABLE_ASPM ?
+			     ASPM_STATE_ALL : link->aspm_enabled;
 
 	/* Setup initial capable state. Will be updated later */
 	link->aspm_capable = link->aspm_support;
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index d797df6e5f3e..a50174619c20 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -6250,3 +6250,14 @@ static void pci_fixup_d3cold_delay_1sec(struct pci_dev *pdev)
 	pdev->d3cold_delay = 1000;
 }
 DECLARE_PCI_FIXUP_FINAL(0x5555, 0x0004, pci_fixup_d3cold_delay_1sec);
+/*
+ * Device [8086:9a09]
+ * BIOS may not be able to access config space of devices under VMD domain, so
+ * it relies on software to enable ASPM for links under VMD.
+ */
+static void pci_fixup_enable_aspm(struct pci_dev *pdev)
+{
+	pdev->dev_flags |= PCI_DEV_FLAGS_ENABLE_ASPM;
+}
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x9a09, pci_fixup_enable_aspm);
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0xa0b0, pci_fixup_enable_aspm);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 7ab0d13672da..59a4177ceddc 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -245,6 +245,8 @@ enum pci_dev_flags {
 	PCI_DEV_FLAGS_NO_RELAXED_ORDERING = (__force pci_dev_flags_t) (1 << 11),
 	/* Device does honor MSI masking despite saying otherwise */
 	PCI_DEV_FLAGS_HAS_MSI_MASKING = (__force pci_dev_flags_t) (1 << 12),
+	/* Enable ASPM regardless of how LnkCtl is programmed */
+	PCI_DEV_FLAGS_ENABLE_ASPM = (__force pci_dev_flags_t) (1 << 13),
 };
 
 enum pci_irq_reroute_variant {
-- 
2.34.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #5: Type: text/x-diff; name=0002-PCI-ASPM-Save-L1-PM-Substates-Capability-for-suspend.patch, Size: 4395 bytes --]

From 365ead85658978363da2467212647a5109c093f8 Mon Sep 17 00:00:00 2001
From: Vidya Sagar <vidyas@nvidia.com>
Date: Tue, 13 Sep 2022 18:48:22 +0530
Subject: [PATCH 2/5] PCI/ASPM: Save L1 PM Substates Capability for
 suspend/resume

Previously the L1 PM Substates Control Registers (CTL1 and CTL2) weren't
saved and restored during suspend/resume leading to the L1 PM Substates
configuration being lost post-resume.

Save the L1 PM Substates Control Registers so that the configuration is
retained post-resume.

[bhelgaas: drop pci_is_pcie() testing; we can rely on pci_configure_ltr()
having already done that]
Link: https://lore.kernel.org/r/20220913131822.16557-3-vidyas@nvidia.com
Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
(cherry picked from commit 4ff116d0d5fd8a025604b0802d93a2d5f4e465d1)
---
 drivers/pci/pci.c       |  7 +++++++
 drivers/pci/pci.h       |  4 ++++
 drivers/pci/pcie/aspm.c | 37 +++++++++++++++++++++++++++++++++++++
 3 files changed, 48 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index c3585229c12a..b80e2a3a085d 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1798,6 +1798,7 @@ int pci_save_state(struct pci_dev *dev)
 		return i;
 
 	pci_save_ltr_state(dev);
+	pci_save_aspm_l1ss_state(dev);
 	pci_save_dpc_state(dev);
 	pci_save_aer_state(dev);
 	pci_save_ptm_state(dev);
@@ -1903,6 +1904,7 @@ void pci_restore_state(struct pci_dev *dev)
 	 * LTR itself (in the PCIe capability).
 	 */
 	pci_restore_ltr_state(dev);
+	pci_restore_aspm_l1ss_state(dev);
 
 	pci_restore_pcie_state(dev);
 	pci_restore_pasid_state(dev);
@@ -3613,6 +3615,11 @@ void pci_allocate_cap_save_buffers(struct pci_dev *dev)
 	if (error)
 		pci_err(dev, "unable to allocate suspend buffer for LTR\n");
 
+	error = pci_add_ext_cap_save_buffer(dev, PCI_EXT_CAP_ID_L1SS,
+					    2 * sizeof(u32));
+	if (error)
+		pci_err(dev, "unable to allocate suspend buffer for ASPM-L1SS\n");
+
 	pci_allocate_vc_save_buffers(dev);
 }
 
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index e9750b1b19ba..5eb65f03b5e8 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -573,11 +573,15 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev);
 void pcie_aspm_exit_link_state(struct pci_dev *pdev);
 void pcie_aspm_pm_state_change(struct pci_dev *pdev, bool locked);
 void pcie_aspm_powersave_config_link(struct pci_dev *pdev);
+void pci_save_aspm_l1ss_state(struct pci_dev *dev);
+void pci_restore_aspm_l1ss_state(struct pci_dev *dev);
 #else
 static inline void pcie_aspm_init_link_state(struct pci_dev *pdev) { }
 static inline void pcie_aspm_exit_link_state(struct pci_dev *pdev) { }
 static inline void pcie_aspm_pm_state_change(struct pci_dev *pdev, bool locked) { }
 static inline void pcie_aspm_powersave_config_link(struct pci_dev *pdev) { }
+static inline void pci_save_aspm_l1ss_state(struct pci_dev *dev) { }
+static inline void pci_restore_aspm_l1ss_state(struct pci_dev *dev) { }
 #endif
 
 #ifdef CONFIG_PCIE_ECRC
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 718e0569321c..840da5be765e 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -714,6 +714,43 @@ static void pcie_config_aspm_l1ss(struct pcie_link_state *link, u32 state)
 				       PCI_L1SS_CTL1_L1SS_MASK, val);
 }
 
+void pci_save_aspm_l1ss_state(struct pci_dev *dev)
+{
+	struct pci_cap_saved_state *save_state;
+	u16 l1ss = dev->l1ss;
+	u32 *cap;
+
+	if (!l1ss)
+		return;
+
+	save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_L1SS);
+	if (!save_state)
+		return;
+
+	cap = (u32 *)&save_state->cap.data[0];
+	pci_read_config_dword(dev, l1ss + PCI_L1SS_CTL2, cap++);
+	pci_read_config_dword(dev, l1ss + PCI_L1SS_CTL1, cap++);
+}
+
+void pci_restore_aspm_l1ss_state(struct pci_dev *dev)
+{
+	struct pci_cap_saved_state *save_state;
+	u32 *cap, ctl1, ctl2;
+	u16 l1ss = dev->l1ss;
+
+	if (!l1ss)
+		return;
+
+	save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_L1SS);
+	if (!save_state)
+		return;
+
+	cap = (u32 *)&save_state->cap.data[0];
+	ctl2 = *cap++;
+	ctl1 = *cap;
+	aspm_program_l1ss(dev, ctl1, ctl2);
+}
+
 static void pcie_config_aspm_dev(struct pci_dev *pdev, u32 val)
 {
 	pcie_capability_clear_and_set_word(pdev, PCI_EXP_LNKCTL,
-- 
2.34.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #6: Type: text/x-diff; name=0001-PCI-ASPM-Refactor-L1-PM-Substates-Control-Register-p.patch, Size: 4995 bytes --]

From 78606e392a1ae2b2ea825c263f68c79d3c00947d Mon Sep 17 00:00:00 2001
From: Vidya Sagar <vidyas@nvidia.com>
Date: Tue, 13 Sep 2022 18:48:21 +0530
Subject: [PATCH 1/5] PCI/ASPM: Refactor L1 PM Substates Control Register
 programming

Refactor the code to extract the common code to program Control
Registers 1 and 2 of the L1 PM Substates capability to a new function
aspm_program_l1ss() and call it for both parent and child devices.

[bhelgaas: squash in update to preserve fields we're not updating from
https://lore.kernel.org/r/36fa13c5-e0f8-022f-77f7-7908e4df98b8@nvidia.com]
Link: https://lore.kernel.org/r/20220913131822.16557-2-vidyas@nvidia.com
Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
(cherry picked from commit 5e85eba6f50dc288c22083a7e213152bcc4b8208)
---
 drivers/pci/pcie/aspm.c | 80 ++++++++++++++++++++---------------------
 1 file changed, 40 insertions(+), 40 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index bc0bd86695ec..718e0569321c 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -426,6 +426,31 @@ static void pcie_aspm_check_latency(struct pci_dev *endpoint)
 	}
 }
 
+static void aspm_program_l1ss(struct pci_dev *dev, u32 ctl1, u32 ctl2)
+{
+	u16 l1ss = dev->l1ss;
+	u32 l1_2_enable;
+
+	/*
+	 * Per PCIe r6.0, sec 5.5.4, T_POWER_ON in PCI_L1SS_CTL2 must be
+	 * programmed prior to setting the L1.2 enable bits in PCI_L1SS_CTL1.
+	 */
+	pci_write_config_dword(dev, l1ss + PCI_L1SS_CTL2, ctl2);
+
+	/*
+	 * In addition, Common_Mode_Restore_Time and LTR_L1.2_THRESHOLD in
+	 * PCI_L1SS_CTL1 must be programmed *before* setting the L1.2
+	 * enable bits, even though they're all in PCI_L1SS_CTL1.
+	 */
+	l1_2_enable = ctl1 & PCI_L1SS_CTL1_L1_2_MASK;
+	ctl1 &= ~PCI_L1SS_CTL1_L1_2_MASK;
+
+	pci_write_config_dword(dev, l1ss + PCI_L1SS_CTL1, ctl1);
+	if (l1_2_enable)
+		pci_write_config_dword(dev, l1ss + PCI_L1SS_CTL1,
+				       ctl1 | l1_2_enable);
+}
+
 /* Calculate L1.2 PM substate timing parameters */
 static void aspm_calc_l12_info(struct pcie_link_state *link,
 				u32 parent_l1ss_cap, u32 child_l1ss_cap)
@@ -435,7 +460,6 @@ static void aspm_calc_l12_info(struct pcie_link_state *link,
 	u32 t_common_mode, t_power_on, l1_2_threshold, scale, value;
 	u32 ctl1 = 0, ctl2 = 0;
 	u32 pctl1, pctl2, cctl1, cctl2;
-	u32 pl1_2_enables, cl1_2_enables;
 
 	/* Choose the greater of the two Port Common_Mode_Restore_Times */
 	val1 = FIELD_GET(PCI_L1SS_CAP_CM_RESTORE_TIME, parent_l1ss_cap);
@@ -485,45 +509,21 @@ static void aspm_calc_l12_info(struct pcie_link_state *link,
 	    ctl2 == pctl2 && ctl2 == cctl2)
 		return;
 
-	/* Disable L1.2 while updating.  See PCIe r5.0, sec 5.5.4, 7.8.3.3 */
-	pl1_2_enables = pctl1 & PCI_L1SS_CTL1_L1_2_MASK;
-	cl1_2_enables = cctl1 & PCI_L1SS_CTL1_L1_2_MASK;
-
-	if (pl1_2_enables || cl1_2_enables) {
-		pci_clear_and_set_config_dword(child,
-					       child->l1ss + PCI_L1SS_CTL1,
-					       PCI_L1SS_CTL1_L1_2_MASK, 0);
-		pci_clear_and_set_config_dword(parent,
-					       parent->l1ss + PCI_L1SS_CTL1,
-					       PCI_L1SS_CTL1_L1_2_MASK, 0);
-	}
-
-	/* Program T_POWER_ON times in both ports */
-	pci_write_config_dword(parent, parent->l1ss + PCI_L1SS_CTL2, ctl2);
-	pci_write_config_dword(child, child->l1ss + PCI_L1SS_CTL2, ctl2);
-
-	/* Program Common_Mode_Restore_Time in upstream device */
-	pci_clear_and_set_config_dword(parent, parent->l1ss + PCI_L1SS_CTL1,
-				       PCI_L1SS_CTL1_CM_RESTORE_TIME, ctl1);
-
-	/* Program LTR_L1.2_THRESHOLD time in both ports */
-	pci_clear_and_set_config_dword(parent, parent->l1ss + PCI_L1SS_CTL1,
-				       PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
-				       PCI_L1SS_CTL1_LTR_L12_TH_SCALE,
-				       ctl1);
-	pci_clear_and_set_config_dword(child, child->l1ss + PCI_L1SS_CTL1,
-				       PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
-				       PCI_L1SS_CTL1_LTR_L12_TH_SCALE,
-				       ctl1);
-
-	if (pl1_2_enables || cl1_2_enables) {
-		pci_clear_and_set_config_dword(parent,
-					       parent->l1ss + PCI_L1SS_CTL1, 0,
-					       pl1_2_enables);
-		pci_clear_and_set_config_dword(child,
-					       child->l1ss + PCI_L1SS_CTL1, 0,
-					       cl1_2_enables);
-	}
+	pctl1 &= ~(PCI_L1SS_CTL1_CM_RESTORE_TIME |
+		   PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
+		   PCI_L1SS_CTL1_LTR_L12_TH_SCALE);
+	pctl1 |= (ctl1 & (PCI_L1SS_CTL1_CM_RESTORE_TIME |
+			  PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
+			  PCI_L1SS_CTL1_LTR_L12_TH_SCALE));
+	aspm_program_l1ss(parent, pctl1, ctl2);
+
+	cctl1 &= ~(PCI_L1SS_CTL1_CM_RESTORE_TIME |
+		   PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
+		   PCI_L1SS_CTL1_LTR_L12_TH_SCALE);
+	cctl1 |= (ctl1 & (PCI_L1SS_CTL1_CM_RESTORE_TIME |
+			  PCI_L1SS_CTL1_LTR_L12_TH_VALUE |
+			  PCI_L1SS_CTL1_LTR_L12_TH_SCALE));
+	aspm_program_l1ss(child, cctl1, ctl2);
 }
 
 static void aspm_l1ss_init(struct pcie_link_state *link)
-- 
2.34.1

Re: My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

[Kai-Heng Feng]

Hi Kenneth,

On Tue, Mar 12, 2024 at 10:37 AM Kenneth R. Crudup <kenny@panix.com> wrote:
>
>
> On Sat, 4 Nov 2023, Kenneth R. Crudup wrote:
>
> > The only release kernel that was able to get this laptop to fully get into
> > low-power (unfortunately only s0ix) was the Ubuntu-6.2.0- ... series from Ubuntu
>
> > I'd bisected it to the following commits:
> > 4ff116d0d5fd PCI/ASPM: Save L1 PM Substates Capability for suspend/resume
> > 5e85eba6f50d PCI/ASPM: Refactor L1 PM Substates Control Register programming
> > 1a0102a08f20 UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under VMD domain
> > 47c7bfd31514 UBUNTU: SAUCE: PCI/ASPM: Enable LTR for endpoints behind VMD
> > 154d48da2c57 UBUNTU: SAUCE: vmd: fixup bridge ASPM by driver name instead
>
> Since (for what I'm sure is a good reason ... I hope :) ) this has yet to make
> it into mainline, here's the set of commits refactored for v6.8; maybe someone
> scanning the archives for a solution to their Dell draining too much power can
> use them.
>
> But is there anything I can do to help these go in? I saw that "Refactor L1
> PM Substates Control Register programming" is still reverted, is that still
> an issue on the machine it affected?

Let me work on this.

I think both VMD and Thunderbolt devices need ASPM enabled by default
regardless of BIOS setting, but I am not sure if PCI folks will like
the idea.

Kai-Heng

>
>         -Kenny
>
> --
> Kenneth R. Crudup / Sr. SW Engineer, Scott County Consulting, Orange County CA

Re: My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

[Kenneth R. Crudup]

On Sat, 4 Nov 2023, Kenneth R. Crudup wrote:

> I'd bisected it to the following commits (in this order):
> 4ff116d0d5fd PCI/ASPM: Save L1 PM Substates Capability for suspend/resume
> 5e85eba6f50d PCI/ASPM: Refactor L1 PM Substates Control Register programming

So the good news is the above two have made it into (the recently-released) 6.9!

Now I'm rooting for these last three:

> 1a0102a08f20 UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under VMD domain
> 47c7bfd31514 UBUNTU: SAUCE: PCI/ASPM: Enable LTR for endpoints behind VMD
> 154d48da2c57 UBUNTU: SAUCE: vmd: fixup bridge ASPM by driver name instead

Applying (refactored for 6.8+ versions) of these last three enable full power
savings for 6.9 .

        -Kenny, fingers crossed

-- 
Kenneth R. Crudup / Sr. SW Engineer, Scott County Consulting, Orange County CA

Re: My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

[Kenneth Crudup]

[-- Attachment #1: Type: text/plain, Size: 1822 bytes --]


No joy yet for 6.10, so here's my patches, maybe it'll save someone some 
time (they're pretty much the same as 6.9) .

Fingers crossed for 6.11!

-Kenny

On 3/21/24 03:12, Kai-Heng Feng wrote:
> Hi Kenneth,
> 
> On Tue, Mar 12, 2024 at 10:37 AM Kenneth R. Crudup <kenny@panix.com> wrote:
>>
>>
>> On Sat, 4 Nov 2023, Kenneth R. Crudup wrote:
>>
>>> The only release kernel that was able to get this laptop to fully get into
>>> low-power (unfortunately only s0ix) was the Ubuntu-6.2.0- ... series from Ubuntu
>>
>>> I'd bisected it to the following commits:
>>> 4ff116d0d5fd PCI/ASPM: Save L1 PM Substates Capability for suspend/resume
>>> 5e85eba6f50d PCI/ASPM: Refactor L1 PM Substates Control Register programming
>>> 1a0102a08f20 UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under VMD domain
>>> 47c7bfd31514 UBUNTU: SAUCE: PCI/ASPM: Enable LTR for endpoints behind VMD
>>> 154d48da2c57 UBUNTU: SAUCE: vmd: fixup bridge ASPM by driver name instead
>>
>> Since (for what I'm sure is a good reason ... I hope :) ) this has yet to make
>> it into mainline, here's the set of commits refactored for v6.8; maybe someone
>> scanning the archives for a solution to their Dell draining too much power can
>> use them.
>>
>> But is there anything I can do to help these go in? I saw that "Refactor L1
>> PM Substates Control Register programming" is still reverted, is that still
>> an issue on the machine it affected?
> 
> Let me work on this.
> 
> I think both VMD and Thunderbolt devices need ASPM enabled by default
> regardless of BIOS setting, but I am not sure if PCI folks will like
> the idea.
> 
> Kai-Heng
> 
>>
>>          -Kenny
>>
>> --
>> Kenneth R. Crudup / Sr. SW Engineer, Scott County Consulting, Orange County CA
> 

-- 
Kenneth R. Crudup / Sr. SW Engineer, Scott County Consulting, Orange 
County CA

[-- Attachment #2: 0003-UBUNTU-SAUCE-vmd-fixup-bridge-ASPM-by-driver-name-in.patch --]
[-- Type: text/x-patch, Size: 3018 bytes --]

From 94709118d1134ddea7ed5586190de9c439f913bd Mon Sep 17 00:00:00 2001
From: You-Sheng Yang <vicamo.yang@canonical.com>
Date: Mon, 11 Apr 2022 17:24:34 +0800
Subject: [PATCH 3/3] UBUNTU: SAUCE: vmd: fixup bridge ASPM by driver name
 instead

BugLink: https://bugs.launchpad.net/bugs/1942160

Additional VMD bridge IDs needed for new Alder Lake platforms, but
actually there is no a complete list for them. Here we match bridge
devices if they're directly attached to a VMD controller instead.

Signed-off-by: You-Sheng Yang <vicamo.yang@canonical.com>
Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
(cherry picked from commit 154d48da2c57514e4b5dadc7b8c70a4edb550981)
---
 drivers/pci/quirks.c | 32 +++++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 269f5325d1a1..94be350caa71 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -6274,23 +6274,36 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_GLI, 0x9750, pci_mask_replay_timer_timeout
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_GLI, 0x9755, pci_mask_replay_timer_timeout);
 #endif
 /*
- * Device [8086:9a09], [8086:a0b0] and [8086:a0bc]
  * BIOS may not be able to access config space of devices under VMD domain, so
  * it relies on software to enable ASPM for links under VMD.
  */
-static const struct pci_device_id vmd_bridge_tbl[] = {
-	{ PCI_VDEVICE(INTEL, 0x9a09) },
-	{ PCI_VDEVICE(INTEL, 0xa0b0) },
-	{ PCI_VDEVICE(INTEL, 0xa0bc) },
-	{ }
-};
+static bool pci_fixup_is_vmd_bridge(struct pci_dev *pdev)
+{
+	struct pci_bus *bus = pdev->bus;
+	struct device *dev;
+	struct pci_driver *pdrv;
+
+	if (!pci_is_root_bus(bus))
+		return false;
+
+	dev = bus->bridge->parent;
+	if (dev == NULL)
+		return false;
+
+	pdrv = pci_dev_driver(to_pci_dev(dev));
+	if (pdrv == NULL || strcmp("vmd", pdrv->name))
+		return false;
+
+	return true;
+}
 
 static void pci_fixup_enable_aspm(struct pci_dev *pdev)
 {
-	if (!pci_match_id(vmd_bridge_tbl, pdev))
+	if (!pci_fixup_is_vmd_bridge(pdev))
 		return;
 
 	pdev->dev_flags |= PCI_DEV_FLAGS_ENABLE_ASPM;
+	pci_info(pdev, "enable ASPM for pci bridge behind vmd");
 }
 DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_VENDOR_ID_INTEL, PCI_ANY_ID,
 			       PCI_CLASS_BRIDGE_PCI, 8, pci_fixup_enable_aspm);
@@ -6305,7 +6318,7 @@ static void pci_fixup_enable_vmd_nvme_ltr(struct pci_dev *pdev)
 	if (!parent)
 		return;
 
-	if (!pci_match_id(vmd_bridge_tbl, parent))
+	if (!pci_fixup_is_vmd_bridge(parent))
 		return;
 
 	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_LTR);
@@ -6323,6 +6336,7 @@ static void pci_fixup_enable_vmd_nvme_ltr(struct pci_dev *pdev)
 	/* 3145728ns, i.e. 0x300000ns */
 	pci_write_config_word(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, 0x1003);
 	pci_write_config_word(pdev, pos + PCI_LTR_MAX_NOSNOOP_LAT, 0x1003);
+	pci_info(pdev, "enable LTR for nvme behind vmd");
 }
 DECLARE_PCI_FIXUP_CLASS_EARLY(PCI_ANY_ID, PCI_ANY_ID,
 			      PCI_CLASS_STORAGE_EXPRESS, 0, pci_fixup_enable_vmd_nvme_ltr);
-- 
2.34.1


[-- Attachment #3: 0002-UBUNTU-SAUCE-PCI-ASPM-Enable-LTR-for-endpoints-behin.patch --]
[-- Type: text/x-patch, Size: 2883 bytes --]

From 36243026bd7be560a61b8e09c811eb481d546e3d Mon Sep 17 00:00:00 2001
From: Kai-Heng Feng <kai.heng.feng@canonical.com>
Date: Mon, 11 Apr 2022 17:24:33 +0800
Subject: [PATCH 2/3] UBUNTU: SAUCE: PCI/ASPM: Enable LTR for endpoints behind
 VMD

BugLink: https://bugs.launchpad.net/bugs/1942160

In addition to ASPM, LTR also needs to be programmed with a reasonable
value to let PCIe link reaches L1.2.

For now, program a hardcoded value that is used under Windows.

While at it, consolidate ASPM and LTR enabling logic to share a same pci
device table.

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
(cherry picked from commit 47c7bfd31514e7b54a1f830f7707297aebbb8679)
---
 drivers/pci/quirks.c | 48 +++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 45 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index eae257c69255..269f5325d1a1 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -6274,13 +6274,55 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_GLI, 0x9750, pci_mask_replay_timer_timeout
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_GLI, 0x9755, pci_mask_replay_timer_timeout);
 #endif
 /*
- * Device [8086:9a09]
+ * Device [8086:9a09], [8086:a0b0] and [8086:a0bc]
  * BIOS may not be able to access config space of devices under VMD domain, so
  * it relies on software to enable ASPM for links under VMD.
  */
+static const struct pci_device_id vmd_bridge_tbl[] = {
+	{ PCI_VDEVICE(INTEL, 0x9a09) },
+	{ PCI_VDEVICE(INTEL, 0xa0b0) },
+	{ PCI_VDEVICE(INTEL, 0xa0bc) },
+	{ }
+};
+
 static void pci_fixup_enable_aspm(struct pci_dev *pdev)
 {
+	if (!pci_match_id(vmd_bridge_tbl, pdev))
+		return;
+
 	pdev->dev_flags |= PCI_DEV_FLAGS_ENABLE_ASPM;
 }
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x9a09, pci_fixup_enable_aspm);
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0xa0b0, pci_fixup_enable_aspm);
+DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_VENDOR_ID_INTEL, PCI_ANY_ID,
+			       PCI_CLASS_BRIDGE_PCI, 8, pci_fixup_enable_aspm);
+
+static void pci_fixup_enable_vmd_nvme_ltr(struct pci_dev *pdev)
+{
+	struct pci_dev *parent;
+	int pos;
+	u16 val;
+
+	parent = pci_upstream_bridge(pdev);
+	if (!parent)
+		return;
+
+	if (!pci_match_id(vmd_bridge_tbl, parent))
+		return;
+
+	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_LTR);
+	if (!pos)
+		return;
+
+	pci_read_config_word(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, &val);
+	if (val)
+		return;
+
+	pci_read_config_word(pdev, pos + PCI_LTR_MAX_NOSNOOP_LAT, &val);
+	if (val)
+		return;
+
+	/* 3145728ns, i.e. 0x300000ns */
+	pci_write_config_word(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, 0x1003);
+	pci_write_config_word(pdev, pos + PCI_LTR_MAX_NOSNOOP_LAT, 0x1003);
+}
+DECLARE_PCI_FIXUP_CLASS_EARLY(PCI_ANY_ID, PCI_ANY_ID,
+			      PCI_CLASS_STORAGE_EXPRESS, 0, pci_fixup_enable_vmd_nvme_ltr);
-- 
2.34.1


[-- Attachment #4: 0001-UBUNTU-SAUCE-PCI-ASPM-Enable-ASPM-for-links-under-VM.patch --]
[-- Type: text/x-patch, Size: 3206 bytes --]

From b09cbedbf37a9651e3bddb9cacb338e6f8f49a81 Mon Sep 17 00:00:00 2001
From: Kai-Heng Feng <kai.heng.feng@canonical.com>
Date: Mon, 11 Apr 2022 17:24:32 +0800
Subject: [PATCH 1/3] UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under VMD
 domain

BugLink: https://bugs.launchpad.net/bugs/1942160

New Intel laptops with VMD cannot reach deeper power saving state,
renders very short battery time.

As BIOS may not be able to program the config space for devices under
VMD domain, ASPM needs to be programmed manually by software. This is
also the case under Windows.

The VMD controller itself is a root complex integrated endpoint that
doesn't have ASPM capability, so we can't propagate the ASPM settings to
devices under it. Hence, simply apply ASPM_STATE_ALL to the links under
VMD domain, unsupported states will be cleared out anyway.

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
(cherry picked from commit 1a0102a08f206149d9abd56c2b28877c878b5526)
---
 drivers/pci/pcie/aspm.c |  3 ++-
 drivers/pci/quirks.c    | 11 +++++++++++
 include/linux/pci.h     |  2 ++
 3 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index cee2365e54b8..ea96ad059814 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -830,7 +830,8 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	aspm_l1ss_init(link);
 
 	/* Save default state */
-	link->aspm_default = link->aspm_enabled;
+	link->aspm_default = parent->dev_flags & PCI_DEV_FLAGS_ENABLE_ASPM ?
+			     PCIE_LINK_STATE_ASPM_ALL : link->aspm_enabled;
 
 	/* Setup initial capable state. Will be updated later */
 	link->aspm_capable = link->aspm_support;
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 568410e64ce6..eae257c69255 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -6273,3 +6273,14 @@ static void pci_mask_replay_timer_timeout(struct pci_dev *pdev)
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_GLI, 0x9750, pci_mask_replay_timer_timeout);
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_GLI, 0x9755, pci_mask_replay_timer_timeout);
 #endif
+/*
+ * Device [8086:9a09]
+ * BIOS may not be able to access config space of devices under VMD domain, so
+ * it relies on software to enable ASPM for links under VMD.
+ */
+static void pci_fixup_enable_aspm(struct pci_dev *pdev)
+{
+	pdev->dev_flags |= PCI_DEV_FLAGS_ENABLE_ASPM;
+}
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x9a09, pci_fixup_enable_aspm);
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0xa0b0, pci_fixup_enable_aspm);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index cafc5ab1cbcb..4b62692638f8 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -245,6 +245,8 @@ enum pci_dev_flags {
 	PCI_DEV_FLAGS_NO_RELAXED_ORDERING = (__force pci_dev_flags_t) (1 << 11),
 	/* Device does honor MSI masking despite saying otherwise */
 	PCI_DEV_FLAGS_HAS_MSI_MASKING = (__force pci_dev_flags_t) (1 << 12),
+	/* Enable ASPM regardless of how LnkCtl is programmed */
+	PCI_DEV_FLAGS_ENABLE_ASPM = (__force pci_dev_flags_t) (1 << 13),
 };
 
 enum pci_irq_reroute_variant {
-- 
2.34.1

Re: My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

[Kai-Heng Feng]

Hi Kenneth,

On Tue, Jul 16, 2024 at 2:27 AM Kenneth Crudup <kenny@panix.com> wrote:
>
>
> No joy yet for 6.10, so here's my patches, maybe it'll save someone some
> time (they're pretty much the same as 6.9) .
>
> Fingers crossed for 6.11!

I forgot to Cc you when I sent the patch [0].

It will be great if you can test it out.

[0] https://lore.kernel.org/linux-pci/20240530085227.91168-1-kai.heng.feng@canonical.com/

Kai-Heng

>
> -Kenny
>
> On 3/21/24 03:12, Kai-Heng Feng wrote:
> > Hi Kenneth,
> >
> > On Tue, Mar 12, 2024 at 10:37 AM Kenneth R. Crudup <kenny@panix.com> wrote:
> >>
> >>
> >> On Sat, 4 Nov 2023, Kenneth R. Crudup wrote:
> >>
> >>> The only release kernel that was able to get this laptop to fully get into
> >>> low-power (unfortunately only s0ix) was the Ubuntu-6.2.0- ... series from Ubuntu
> >>
> >>> I'd bisected it to the following commits:
> >>> 4ff116d0d5fd PCI/ASPM: Save L1 PM Substates Capability for suspend/resume
> >>> 5e85eba6f50d PCI/ASPM: Refactor L1 PM Substates Control Register programming
> >>> 1a0102a08f20 UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under VMD domain
> >>> 47c7bfd31514 UBUNTU: SAUCE: PCI/ASPM: Enable LTR for endpoints behind VMD
> >>> 154d48da2c57 UBUNTU: SAUCE: vmd: fixup bridge ASPM by driver name instead
> >>
> >> Since (for what I'm sure is a good reason ... I hope :) ) this has yet to make
> >> it into mainline, here's the set of commits refactored for v6.8; maybe someone
> >> scanning the archives for a solution to their Dell draining too much power can
> >> use them.
> >>
> >> But is there anything I can do to help these go in? I saw that "Refactor L1
> >> PM Substates Control Register programming" is still reverted, is that still
> >> an issue on the machine it affected?
> >
> > Let me work on this.
> >
> > I think both VMD and Thunderbolt devices need ASPM enabled by default
> > regardless of BIOS setting, but I am not sure if PCI folks will like
> > the idea.
> >
> > Kai-Heng
> >
> >>
> >>          -Kenny
> >>
> >> --
> >> Kenneth R. Crudup / Sr. SW Engineer, Scott County Consulting, Orange County CA
> >
>
> --
> Kenneth R. Crudup / Sr. SW Engineer, Scott County Consulting, Orange
> County CA

Re: My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

[Kenneth Crudup]

OK, I'll test it out. But in the meantime, I've seen this:

----
[E0] 502 /home/kenny> dmesg | fgrep -i aspm
[    0.160156] ACPI FADT declares the system doesn't support PCIe ASPM, 
so disable it
[    0.591366] acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM 
ClockPM Segments MSI EDR HPX-Type3]
[    0.597820] acpi PNP0A08:00: FADT indicates ASPM is unsupported, 
using BIOS configuration
[    1.375136] pci 10000:e0:06.0: enable ASPM for pci bridge behind vmd
[    1.689608] pci 10000:e1:00.0: can't override BIOS ASPM; OS doesn't 
have ASPM control
[16/20:19:35 kenny@xps-9320]
[E0] 503 /home/kenny>
----

IOW, it doesn't appear to override ASPM but I still see power savings.

Gimme a few and I'll let you know how it worked out.

-K

On 7/16/24 18:59, Kai-Heng Feng wrote:
> Hi Kenneth,
> 
> On Tue, Jul 16, 2024 at 2:27 AM Kenneth Crudup <kenny@panix.com> wrote:
>>
>>
>> No joy yet for 6.10, so here's my patches, maybe it'll save someone some
>> time (they're pretty much the same as 6.9) .
>>
>> Fingers crossed for 6.11!
> 
> I forgot to Cc you when I sent the patch [0].
> 
> It will be great if you can test it out.
> 
> [0] https://lore.kernel.org/linux-pci/20240530085227.91168-1-kai.heng.feng@canonical.com/
> 
> Kai-Heng
> 
>>
>> -Kenny
>>
>> On 3/21/24 03:12, Kai-Heng Feng wrote:
>>> Hi Kenneth,
>>>
>>> On Tue, Mar 12, 2024 at 10:37 AM Kenneth R. Crudup <kenny@panix.com> wrote:
>>>>
>>>>
>>>> On Sat, 4 Nov 2023, Kenneth R. Crudup wrote:
>>>>
>>>>> The only release kernel that was able to get this laptop to fully get into
>>>>> low-power (unfortunately only s0ix) was the Ubuntu-6.2.0- ... series from Ubuntu
>>>>
>>>>> I'd bisected it to the following commits:
>>>>> 4ff116d0d5fd PCI/ASPM: Save L1 PM Substates Capability for suspend/resume
>>>>> 5e85eba6f50d PCI/ASPM: Refactor L1 PM Substates Control Register programming
>>>>> 1a0102a08f20 UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under VMD domain
>>>>> 47c7bfd31514 UBUNTU: SAUCE: PCI/ASPM: Enable LTR for endpoints behind VMD
>>>>> 154d48da2c57 UBUNTU: SAUCE: vmd: fixup bridge ASPM by driver name instead
>>>>
>>>> Since (for what I'm sure is a good reason ... I hope :) ) this has yet to make
>>>> it into mainline, here's the set of commits refactored for v6.8; maybe someone
>>>> scanning the archives for a solution to their Dell draining too much power can
>>>> use them.
>>>>
>>>> But is there anything I can do to help these go in? I saw that "Refactor L1
>>>> PM Substates Control Register programming" is still reverted, is that still
>>>> an issue on the machine it affected?
>>>
>>> Let me work on this.
>>>
>>> I think both VMD and Thunderbolt devices need ASPM enabled by default
>>> regardless of BIOS setting, but I am not sure if PCI folks will like
>>> the idea.
>>>
>>> Kai-Heng
>>>
>>>>
>>>>           -Kenny
>>>>
>>>> --
>>>> Kenneth R. Crudup / Sr. SW Engineer, Scott County Consulting, Orange County CA
>>>
>>
>> --
>> Kenneth R. Crudup / Sr. SW Engineer, Scott County Consulting, Orange
>> County CA
> 

-- 
Kenneth R. Crudup / Sr. SW Engineer, Scott County Consulting, Orange 
County CA

Re: My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

[Kenneth Crudup]

So far, so good.

----
[E0] 502 /home/kenny> dmesg | fgrep -i aspm
[    0.156949] ACPI FADT declares the system doesn't support PCIe ASPM, 
so disable it
[    0.590419] acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM 
ClockPM Segments MSI EDR HPX-Type3]
[    0.597007] acpi PNP0A08:00: FADT indicates ASPM is unsupported, 
using BIOS configuration
[    1.402419] pci 10000:e0:06.0: enable ASPM for pci bridge behind vmd
[    1.719570] pci 10000:e1:00.0: BIOS can't program ASPM, let OS control it
[16/21:37:27 kenny@xps-9320]
[E0] 503 /home/kenny> cat /tmp/battery-levels
suspend-in:  Tue, 16 Jul 2024 20:59:44 -0700
/sys/power/wakeup_count:9880
/sys/power/suspend_stats/success:0
/sys/class/power_supply/BAT0/charge_full:3882000
/sys/class/power_supply/BAT0/capacity:56
/sys/class/power_supply/BAT0/voltage_now:11690000
/sys/class/power_supply/BAT0/status:Discharging
/sys/class/power_supply/BAT0/capacity_level:Normal
/sys/devices/system/cpu/cpuidle/low_power_idle_system_residency_us:13549469
/sys/kernel/debug/pmc_core/slp_s0_residency_usec:13549930
----------
suspend-out: Tue, 16 Jul 2024 21:37:15 -0700
/sys/kernel/debug/pmc_core/slp_s0_residency_usec:13549930
/sys/devices/system/cpu/cpuidle/low_power_idle_system_residency_us:13549469
/sys/class/power_supply/BAT0/charge_full:3882000
/sys/class/power_supply/BAT0/capacity:55
/sys/class/power_supply/BAT0/voltage_now:11622000
/sys/class/power_supply/BAT0/status:Discharging
/sys/class/power_supply/BAT0/capacity_level:Normal
/sys/power/suspend_stats/success:1
/sys/power/wakeup_count:9884
==================================================

[16/21:37:49 kenny@xps-9320]
----

I'll have more info in the next few days.

-K

On 7/16/24 20:39, Kenneth Crudup wrote:
> 
> OK, I'll test it out. But in the meantime, I've seen this:
> 
> ----
> [E0] 502 /home/kenny> dmesg | fgrep -i aspm
> [    0.160156] ACPI FADT declares the system doesn't support PCIe ASPM, 
> so disable it
> [    0.591366] acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM 
> ClockPM Segments MSI EDR HPX-Type3]
> [    0.597820] acpi PNP0A08:00: FADT indicates ASPM is unsupported, 
> using BIOS configuration
> [    1.375136] pci 10000:e0:06.0: enable ASPM for pci bridge behind vmd
> [    1.689608] pci 10000:e1:00.0: can't override BIOS ASPM; OS doesn't 
> have ASPM control
> [16/20:19:35 kenny@xps-9320]
> [E0] 503 /home/kenny>
> ----
> 
> IOW, it doesn't appear to override ASPM but I still see power savings.
> 
> Gimme a few and I'll let you know how it worked out.
> 
> -K
> 
> On 7/16/24 18:59, Kai-Heng Feng wrote:
>> Hi Kenneth,
>>
>> On Tue, Jul 16, 2024 at 2:27 AM Kenneth Crudup <kenny@panix.com> wrote:
>>>
>>>
>>> No joy yet for 6.10, so here's my patches, maybe it'll save someone some
>>> time (they're pretty much the same as 6.9) .
>>>
>>> Fingers crossed for 6.11!
>>
>> I forgot to Cc you when I sent the patch [0].
>>
>> It will be great if you can test it out.
>>
>> [0] 
>> https://lore.kernel.org/linux-pci/20240530085227.91168-1-kai.heng.feng@canonical.com/
>>
>> Kai-Heng
>>
>>>
>>> -Kenny
>>>
>>> On 3/21/24 03:12, Kai-Heng Feng wrote:
>>>> Hi Kenneth,
>>>>
>>>> On Tue, Mar 12, 2024 at 10:37 AM Kenneth R. Crudup <kenny@panix.com> 
>>>> wrote:
>>>>>
>>>>>
>>>>> On Sat, 4 Nov 2023, Kenneth R. Crudup wrote:
>>>>>
>>>>>> The only release kernel that was able to get this laptop to fully 
>>>>>> get into
>>>>>> low-power (unfortunately only s0ix) was the Ubuntu-6.2.0- ... 
>>>>>> series from Ubuntu
>>>>>
>>>>>> I'd bisected it to the following commits:
>>>>>> 4ff116d0d5fd PCI/ASPM: Save L1 PM Substates Capability for 
>>>>>> suspend/resume
>>>>>> 5e85eba6f50d PCI/ASPM: Refactor L1 PM Substates Control Register 
>>>>>> programming
>>>>>> 1a0102a08f20 UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under 
>>>>>> VMD domain
>>>>>> 47c7bfd31514 UBUNTU: SAUCE: PCI/ASPM: Enable LTR for endpoints 
>>>>>> behind VMD
>>>>>> 154d48da2c57 UBUNTU: SAUCE: vmd: fixup bridge ASPM by driver name 
>>>>>> instead
>>>>>
>>>>> Since (for what I'm sure is a good reason ... I hope :) ) this has 
>>>>> yet to make
>>>>> it into mainline, here's the set of commits refactored for v6.8; 
>>>>> maybe someone
>>>>> scanning the archives for a solution to their Dell draining too 
>>>>> much power can
>>>>> use them.
>>>>>
>>>>> But is there anything I can do to help these go in? I saw that 
>>>>> "Refactor L1
>>>>> PM Substates Control Register programming" is still reverted, is 
>>>>> that still
>>>>> an issue on the machine it affected?
>>>>
>>>> Let me work on this.
>>>>
>>>> I think both VMD and Thunderbolt devices need ASPM enabled by default
>>>> regardless of BIOS setting, but I am not sure if PCI folks will like
>>>> the idea.
>>>>
>>>> Kai-Heng
>>>>
>>>>>
>>>>>           -Kenny
>>>>>
>>>>> -- 
>>>>> Kenneth R. Crudup / Sr. SW Engineer, Scott County Consulting, 
>>>>> Orange County CA
>>>>
>>>
>>> -- 
>>> Kenneth R. Crudup / Sr. SW Engineer, Scott County Consulting, Orange
>>> County CA
>>
> 

-- 
Kenneth R. Crudup / Sr. SW Engineer, Scott County Consulting, Orange 
County CA

Re: My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

[Kenneth Crudup]

Seems to be working OK, I do think running power states seem to be 
spending more time in pc10/cpuLPI/SysLPI, but that's just 
seat-of-the-pants though.

While-suspended battery drain seems about the same, however:

----
[E130] 518 /home/kenny> cat /tmp/battery-levels
suspend-in:  Wed, 17 Jul 2024 09:33:33 -0700
/sys/power/wakeup_count:46
/sys/power/suspend_stats/success:0
/sys/class/power_supply/BAT0/charge_full:3882000
/sys/class/power_supply/BAT0/capacity:100
/sys/class/power_supply/BAT0/voltage_now:12856000
/sys/class/power_supply/BAT0/status:Full
/sys/class/power_supply/BAT0/capacity_level:Full
/sys/devices/system/cpu/cpuidle/low_power_idle_system_residency_us:0
/sys/kernel/debug/pmc_core/slp_s0_residency_usec:0
----------
suspend-out: Wed, 17 Jul 2024 16:38:37 -0700
/sys/kernel/debug/pmc_core/slp_s0_residency_usec:0
/sys/devices/system/cpu/cpuidle/low_power_idle_system_residency_us:0
/sys/class/power_supply/BAT0/charge_full:3882000
/sys/class/power_supply/BAT0/capacity:89
/sys/class/power_supply/BAT0/voltage_now:12206000
/sys/class/power_supply/BAT0/status:Discharging
/sys/class/power_supply/BAT0/capacity_level:Normal
/sys/power/suspend_stats/success:1
/sys/power/wakeup_count:52
==================================================

[17/16:44:11 kenny@xps-9320]
----

On 7/16/24 18:59, Kai-Heng Feng wrote:
> Hi Kenneth,
> 
> On Tue, Jul 16, 2024 at 2:27 AM Kenneth Crudup <kenny@panix.com> wrote:
>>
>>
>> No joy yet for 6.10, so here's my patches, maybe it'll save someone some
>> time (they're pretty much the same as 6.9) .
>>
>> Fingers crossed for 6.11!
> 
> I forgot to Cc you when I sent the patch [0].
> 
> It will be great if you can test it out.
> 
> [0] https://lore.kernel.org/linux-pci/20240530085227.91168-1-kai.heng.feng@canonical.com/
> 
> Kai-Heng
> 
>>
>> -Kenny
>>
>> On 3/21/24 03:12, Kai-Heng Feng wrote:
>>> Hi Kenneth,
>>>
>>> On Tue, Mar 12, 2024 at 10:37 AM Kenneth R. Crudup <kenny@panix.com> wrote:
>>>>
>>>>
>>>> On Sat, 4 Nov 2023, Kenneth R. Crudup wrote:
>>>>
>>>>> The only release kernel that was able to get this laptop to fully get into
>>>>> low-power (unfortunately only s0ix) was the Ubuntu-6.2.0- ... series from Ubuntu
>>>>
>>>>> I'd bisected it to the following commits:
>>>>> 4ff116d0d5fd PCI/ASPM: Save L1 PM Substates Capability for suspend/resume
>>>>> 5e85eba6f50d PCI/ASPM: Refactor L1 PM Substates Control Register programming
>>>>> 1a0102a08f20 UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under VMD domain
>>>>> 47c7bfd31514 UBUNTU: SAUCE: PCI/ASPM: Enable LTR for endpoints behind VMD
>>>>> 154d48da2c57 UBUNTU: SAUCE: vmd: fixup bridge ASPM by driver name instead
>>>>
>>>> Since (for what I'm sure is a good reason ... I hope :) ) this has yet to make
>>>> it into mainline, here's the set of commits refactored for v6.8; maybe someone
>>>> scanning the archives for a solution to their Dell draining too much power can
>>>> use them.
>>>>
>>>> But is there anything I can do to help these go in? I saw that "Refactor L1
>>>> PM Substates Control Register programming" is still reverted, is that still
>>>> an issue on the machine it affected?
>>>
>>> Let me work on this.
>>>
>>> I think both VMD and Thunderbolt devices need ASPM enabled by default
>>> regardless of BIOS setting, but I am not sure if PCI folks will like
>>> the idea.
>>>
>>> Kai-Heng
>>>
>>>>
>>>>           -Kenny
>>>>
>>>> --
>>>> Kenneth R. Crudup / Sr. SW Engineer, Scott County Consulting, Orange County CA
>>>
>>
>> --
>> Kenneth R. Crudup / Sr. SW Engineer, Scott County Consulting, Orange
>> County CA
> 

-- 
Kenneth R. Crudup / Sr. SW Engineer, Scott County Consulting, Orange 
County CA

Re: My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

[Kenneth Crudup]

So we're on 6.13-rc2 and the patches are getting closer and closer, but 
they still need to be manually added.

The good news is now only (variants of) "PCI/ASPM: Enable LTR for 
endpoints behind VMD" and "PCI/ASPM: Enable ASPM for links under VMD 
domain" are needed.

Any new news on getting those upstreamed?

But I was curious as why I wasn't seeing the "enable LTR for nvme behind 
vmd" message on bootup, and looking at it further it looks like my BIOS 
already sets the "PCI_LTR_MAX_SNOOP_LAT" and "PCI_LTR_MAX_NOSNOOP_LAT" 
values to the ones the patch does, so I'm submitting a modified patch 
that lets you know if it's already been set (via BIOS or elsewhere).

It's not a "real" patch format (I'm running it but haven't committed it 
yet) but I think you should get the gist of what it's doing:

----
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index c5145e74df73..cdb9f34eb67d 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -6336,11 +6336,13 @@ static void pci_fixup_enable_aspm(struct pci_dev 
*pdev)
  DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_VENDOR_ID_INTEL, PCI_ANY_ID,
  			       PCI_CLASS_BRIDGE_PCI, 8, pci_fixup_enable_aspm);

+#define LTR_LATENCY_300US	(0x1003) /* 3145728ns, i.e. 0x300000ns */
+
  static void pci_fixup_enable_vmd_nvme_ltr(struct pci_dev *pdev)
  {
  	struct pci_dev *parent;
-	int pos;
-	u16 val;
+	int pos, ret;
+	u16 val_sn, val_ns;

  	parent = pci_upstream_bridge(pdev);
  	if (!parent)
@@ -6353,18 +6355,21 @@ static void pci_fixup_enable_vmd_nvme_ltr(struct 
pci_dev *pdev)
  	if (!pos)
  		return;

-	pci_read_config_word(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, &val);
-	if (val)
+	ret = pci_read_config_word(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, &val_sn);
+	if (ret || (val_sn && (val_sn != LTR_LATENCY_300US)))
  		return;

-	pci_read_config_word(pdev, pos + PCI_LTR_MAX_NOSNOOP_LAT, &val);
-	if (val)
+	ret = pci_read_config_word(pdev, pos + PCI_LTR_MAX_NOSNOOP_LAT, &val_ns);
+	if (ret || (val_ns && (val_ns != LTR_LATENCY_300US)))
  		return;

-	/* 3145728ns, i.e. 0x300000ns */
-	pci_write_config_word(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, 0x1003);
-	pci_write_config_word(pdev, pos + PCI_LTR_MAX_NOSNOOP_LAT, 0x1003);
-	pci_info(pdev, "enable LTR for nvme behind vmd");
+	if (!val_sn && !val_ns) {
+		ret = pci_write_config_word(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, 
LTR_LATENCY_300US);
+		ret = pci_write_config_word(pdev, pos + PCI_LTR_MAX_NOSNOOP_LAT, 
LTR_LATENCY_300US);
+		pci_info(pdev, "enable LTR for nvme behind vmd");
+	} else {
+		pci_info(pdev, "LTR for nvme behind vmd enabled via BIOS");
+	}
  }
  DECLARE_PCI_FIXUP_CLASS_EARLY(PCI_ANY_ID, PCI_ANY_ID,
  			      PCI_CLASS_STORAGE_EXPRESS, 0, pci_fixup_enable_vmd_nvme_ltr);
----

On 7/17/24 16:46, Kenneth Crudup wrote:
> 
> Seems to be working OK, I do think running power states seem to be 
> spending more time in pc10/cpuLPI/SysLPI, but that's just seat-of-the- 
> pants though.
> 
> While-suspended battery drain seems about the same, however:
> 
> ----
> [E130] 518 /home/kenny> cat /tmp/battery-levels
> suspend-in:  Wed, 17 Jul 2024 09:33:33 -0700
> /sys/power/wakeup_count:46
> /sys/power/suspend_stats/success:0
> /sys/class/power_supply/BAT0/charge_full:3882000
> /sys/class/power_supply/BAT0/capacity:100
> /sys/class/power_supply/BAT0/voltage_now:12856000
> /sys/class/power_supply/BAT0/status:Full
> /sys/class/power_supply/BAT0/capacity_level:Full
> /sys/devices/system/cpu/cpuidle/low_power_idle_system_residency_us:0
> /sys/kernel/debug/pmc_core/slp_s0_residency_usec:0
> ----------
> suspend-out: Wed, 17 Jul 2024 16:38:37 -0700
> /sys/kernel/debug/pmc_core/slp_s0_residency_usec:0
> /sys/devices/system/cpu/cpuidle/low_power_idle_system_residency_us:0
> /sys/class/power_supply/BAT0/charge_full:3882000
> /sys/class/power_supply/BAT0/capacity:89
> /sys/class/power_supply/BAT0/voltage_now:12206000
> /sys/class/power_supply/BAT0/status:Discharging
> /sys/class/power_supply/BAT0/capacity_level:Normal
> /sys/power/suspend_stats/success:1
> /sys/power/wakeup_count:52
> ==================================================
> 
> [17/16:44:11 kenny@xps-9320]
> ----
> 
> On 7/16/24 18:59, Kai-Heng Feng wrote:
>> Hi Kenneth,
>>
>> On Tue, Jul 16, 2024 at 2:27 AM Kenneth Crudup <kenny@panix.com> wrote:
>>>
>>>
>>> No joy yet for 6.10, so here's my patches, maybe it'll save someone some
>>> time (they're pretty much the same as 6.9) .
>>>
>>> Fingers crossed for 6.11!
>>
>> I forgot to Cc you when I sent the patch [0].
>>
>> It will be great if you can test it out.
>>
>> [0] https://lore.kernel.org/linux-pci/20240530085227.91168-1- 
>> kai.heng.feng@canonical.com/
>>
>> Kai-Heng
>>
>>>
>>> -Kenny
>>>
>>> On 3/21/24 03:12, Kai-Heng Feng wrote:
>>>> Hi Kenneth,
>>>>
>>>> On Tue, Mar 12, 2024 at 10:37 AM Kenneth R. Crudup <kenny@panix.com> 
>>>> wrote:
>>>>>
>>>>>
>>>>> On Sat, 4 Nov 2023, Kenneth R. Crudup wrote:
>>>>>
>>>>>> The only release kernel that was able to get this laptop to fully 
>>>>>> get into
>>>>>> low-power (unfortunately only s0ix) was the Ubuntu-6.2.0- ... 
>>>>>> series from Ubuntu
>>>>>
>>>>>> I'd bisected it to the following commits:
>>>>>> 4ff116d0d5fd PCI/ASPM: Save L1 PM Substates Capability for 
>>>>>> suspend/resume
>>>>>> 5e85eba6f50d PCI/ASPM: Refactor L1 PM Substates Control Register 
>>>>>> programming
>>>>>> 1a0102a08f20 UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under 
>>>>>> VMD domain
>>>>>> 47c7bfd31514 UBUNTU: SAUCE: PCI/ASPM: Enable LTR for endpoints 
>>>>>> behind VMD
>>>>>> 154d48da2c57 UBUNTU: SAUCE: vmd: fixup bridge ASPM by driver name 
>>>>>> instead
>>>>>
>>>>> Since (for what I'm sure is a good reason ... I hope :) ) this has 
>>>>> yet to make
>>>>> it into mainline, here's the set of commits refactored for v6.8; 
>>>>> maybe someone
>>>>> scanning the archives for a solution to their Dell draining too 
>>>>> much power can
>>>>> use them.
>>>>>
>>>>> But is there anything I can do to help these go in? I saw that 
>>>>> "Refactor L1
>>>>> PM Substates Control Register programming" is still reverted, is 
>>>>> that still
>>>>> an issue on the machine it affected?
>>>>
>>>> Let me work on this.
>>>>
>>>> I think both VMD and Thunderbolt devices need ASPM enabled by default
>>>> regardless of BIOS setting, but I am not sure if PCI folks will like
>>>> the idea.
>>>>
>>>> Kai-Heng
>>>>
>>>>>
>>>>>           -Kenny
>>>>>
>>>>> -- 
>>>>> Kenneth R. Crudup / Sr. SW Engineer, Scott County Consulting, 
>>>>> Orange County CA
>>>>
>>>
>>> -- 
>>> Kenneth R. Crudup / Sr. SW Engineer, Scott County Consulting, Orange
>>> County CA
>>
> 

-- 
Kenneth R. Crudup / Sr. SW Engineer, Scott County Consulting, Orange 
County CA

Re: My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

[Bjorn Helgaas]

On Wed, Dec 11, 2024 at 03:26:37PM -0800, Kenneth Crudup wrote:
> 
> So we're on 6.13-rc2 and the patches are getting closer and closer, but they
> still need to be manually added.
> 
> The good news is now only (variants of) "PCI/ASPM: Enable LTR for endpoints
> behind VMD" and "PCI/ASPM: Enable ASPM for links under VMD domain" are
> needed.

Do you have lore links for these handy?  I'm not sure exactly what to
look at.

https://lore.kernel.org/linux-pci/?q=s%3A%22Enable+LTR+for+endpoints+behind+VMD%22
didn't find anything.

https://lore.kernel.org/linux-pci/?q=s%3A%22Enable+ASPM+for+links+under+VMD+domain%22
found
https://lore.kernel.org/linux-pci/20200821123222.32093-1-kai.heng.feng@canonical.com/t/#u,
but it's four years old.

Re: My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

[Kenneth Crudup]

On 12/12/24 12:56, Bjorn Helgaas wrote:
> On Wed, Dec 11, 2024 at 03:26:37PM -0800, Kenneth Crudup wrote:
>>
>> So we're on 6.13-rc2 and the patches are getting closer and closer, but they
>> still need to be manually added.
>>
>> The good news is now only (variants of) "PCI/ASPM: Enable LTR for endpoints
>> behind VMD" and "PCI/ASPM: Enable ASPM for links under VMD domain" are
>> needed.
> 
> Do you have lore links for these handy?  I'm not sure exactly what to
> look at.
> 
> https://lore.kernel.org/linux-pci/?q=s%3A%22Enable+LTR+for+endpoints+behind+VMD%22
> didn't find anything.
> 
> https://lore.kernel.org/linux-pci/?q=s%3A%22Enable+ASPM+for+links+under+VMD+domain%22
> found
> https://lore.kernel.org/linux-pci/20200821123222.32093-1-kai.heng.feng@canonical.com/t/#u,
> but it's four years old.
> 

----
On Mon, 6 Nov 2023, Bjorn Helgaas wrote:

 > > I'd bisected it to the following commits (in this order):
 > > 4ff116d0d5fd PCI/ASPM: Save L1 PM Substates Capability for 
suspend/resume
 > > 5e85eba6f50d PCI/ASPM: Refactor L1 PM Substates Control Register 
programming
 > > 1a0102a08f20 UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under 
VMD domain
 > > 47c7bfd31514 UBUNTU: SAUCE: PCI/ASPM: Enable LTR for endpoints 
behind VMD
 > > 154d48da2c57 UBUNTU: SAUCE: vmd: fixup bridge ASPM by driver name 
instead

 > Thanks for these.  You don't happen to have URLs for those Ubuntu
 > commits, do you?

https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/lunar/commit/?id=1a0102a08f206149d9abd56c2b28877c
878b5526
https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/lunar/commit/?id=47c7bfd31514e7b54a1f830f7707297a
ebbb8679
https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/lunar/commit/?id=154d48da2c57514e4b5dadc7b8c70a4e
db550981
----

-Kenny


-- 
Kenneth R. Crudup / Sr. SW Engineer, Scott County Consulting, Orange 
County CA

Re: My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

[Kenneth Crudup]

[-- Attachment #1: Type: text/plain, Size: 2273 bytes --]


Attached are all three patches I use (against 6.13-rc2, et al.). I've 
included "UBUNTU: SAUCE: vmd: fixup bridge ASPM by driver name instead" 
but I'm thinking that one has been superceded in Linus' master.

On 12/12/24 15:04, Kenneth Crudup wrote:
> 
> 
> On 12/12/24 12:56, Bjorn Helgaas wrote:
>> On Wed, Dec 11, 2024 at 03:26:37PM -0800, Kenneth Crudup wrote:
>>>
>>> So we're on 6.13-rc2 and the patches are getting closer and closer, 
>>> but they
>>> still need to be manually added.
>>>
>>> The good news is now only (variants of) "PCI/ASPM: Enable LTR for 
>>> endpoints
>>> behind VMD" and "PCI/ASPM: Enable ASPM for links under VMD domain" are
>>> needed.
>>
>> Do you have lore links for these handy?  I'm not sure exactly what to
>> look at.
>>
>> https://lore.kernel.org/linux-pci/? 
>> q=s%3A%22Enable+LTR+for+endpoints+behind+VMD%22
>> didn't find anything.
>>
>> https://lore.kernel.org/linux-pci/? 
>> q=s%3A%22Enable+ASPM+for+links+under+VMD+domain%22
>> found
>> https://lore.kernel.org/linux-pci/20200821123222.32093-1- 
>> kai.heng.feng@canonical.com/t/#u,
>> but it's four years old.
>>
> 
> ----
> On Mon, 6 Nov 2023, Bjorn Helgaas wrote:
> 
>  > > I'd bisected it to the following commits (in this order):
>  > > 4ff116d0d5fd PCI/ASPM: Save L1 PM Substates Capability for suspend/ 
> resume
>  > > 5e85eba6f50d PCI/ASPM: Refactor L1 PM Substates Control Register 
> programming
>  > > 1a0102a08f20 UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under 
> VMD domain
>  > > 47c7bfd31514 UBUNTU: SAUCE: PCI/ASPM: Enable LTR for endpoints 
> behind VMD
>  > > 154d48da2c57 UBUNTU: SAUCE: vmd: fixup bridge ASPM by driver name 
> instead
> 
>  > Thanks for these.  You don't happen to have URLs for those Ubuntu
>  > commits, do you?
> 
> https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/ 
> lunar/commit/?id=1a0102a08f206149d9abd56c2b28877c
> 878b5526
> https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/ 
> lunar/commit/?id=47c7bfd31514e7b54a1f830f7707297a
> ebbb8679
> https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/ 
> lunar/commit/?id=154d48da2c57514e4b5dadc7b8c70a4e
> db550981
> ----
> 
> -Kenny
> 
> 

-- 
Kenneth R. Crudup / Sr. SW Engineer, Scott County Consulting, Orange 
County CA

[-- Attachment #2: 0001-UBUNTU-SAUCE-PCI-ASPM-Enable-ASPM-for-links-under-VM.patch --]
[-- Type: text/x-patch, Size: 3202 bytes --]

From 99e5aa6c493a5af1f78fab9ae5e90673bc5a5bf4 Mon Sep 17 00:00:00 2001
From: Kai-Heng Feng <kai.heng.feng@canonical.com>
Date: Mon, 11 Apr 2022 17:24:32 +0800
Subject: [PATCH] UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under VMD
 domain

BugLink: https://bugs.launchpad.net/bugs/1942160

New Intel laptops with VMD cannot reach deeper power saving state,
renders very short battery time.

As BIOS may not be able to program the config space for devices under
VMD domain, ASPM needs to be programmed manually by software. This is
also the case under Windows.

The VMD controller itself is a root complex integrated endpoint that
doesn't have ASPM capability, so we can't propagate the ASPM settings to
devices under it. Hence, simply apply ASPM_STATE_ALL to the links under
VMD domain, unsupported states will be cleared out anyway.

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
(cherry picked from commit 1a0102a08f206149d9abd56c2b28877c878b5526)
---
 drivers/pci/pcie/aspm.c |  3 ++-
 drivers/pci/quirks.c    | 11 +++++++++++
 include/linux/pci.h     |  2 ++
 3 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index cee2365e54b8..ea96ad059814 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -830,7 +830,8 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	aspm_l1ss_init(link);
 
 	/* Save default state */
-	link->aspm_default = link->aspm_enabled;
+	link->aspm_default = parent->dev_flags & PCI_DEV_FLAGS_ENABLE_ASPM ?
+			     PCIE_LINK_STATE_ASPM_ALL : link->aspm_enabled;
 
 	/* Setup initial capable state. Will be updated later */
 	link->aspm_capable = link->aspm_support;
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index a2ce4e08edf5..5c9f0bd71558 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -6277,3 +6277,14 @@ static void pci_mask_replay_timer_timeout(struct pci_dev *pdev)
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_GLI, 0x9750, pci_mask_replay_timer_timeout);
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_GLI, 0x9755, pci_mask_replay_timer_timeout);
 #endif
+/*
+ * Device [8086:9a09]
+ * BIOS may not be able to access config space of devices under VMD domain, so
+ * it relies on software to enable ASPM for links under VMD.
+ */
+static void pci_fixup_enable_aspm(struct pci_dev *pdev)
+{
+	pdev->dev_flags |= PCI_DEV_FLAGS_ENABLE_ASPM;
+}
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x9a09, pci_fixup_enable_aspm);
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0xa0b0, pci_fixup_enable_aspm);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 4cf89a4b4cbc..a8851b3ed679 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -245,6 +245,8 @@ enum pci_dev_flags {
 	PCI_DEV_FLAGS_NO_RELAXED_ORDERING = (__force pci_dev_flags_t) (1 << 11),
 	/* Device does honor MSI masking despite saying otherwise */
 	PCI_DEV_FLAGS_HAS_MSI_MASKING = (__force pci_dev_flags_t) (1 << 12),
+	/* Enable ASPM regardless of how LnkCtl is programmed */
+	PCI_DEV_FLAGS_ENABLE_ASPM = (__force pci_dev_flags_t) (1 << 13),
 };
 
 enum pci_irq_reroute_variant {
-- 
2.43.0


[-- Attachment #3: 0001-UBUNTU-SAUCE-PCI-ASPM-Enable-LTR-for-endpoints-behin.patch --]
[-- Type: text/x-patch, Size: 2878 bytes --]

From ccb1c132dc27950b133831e3e2506a874e9f7da6 Mon Sep 17 00:00:00 2001
From: Kai-Heng Feng <kai.heng.feng@canonical.com>
Date: Mon, 11 Apr 2022 17:24:33 +0800
Subject: [PATCH] UBUNTU: SAUCE: PCI/ASPM: Enable LTR for endpoints behind VMD

BugLink: https://bugs.launchpad.net/bugs/1942160

In addition to ASPM, LTR also needs to be programmed with a reasonable
value to let PCIe link reaches L1.2.

For now, program a hardcoded value that is used under Windows.

While at it, consolidate ASPM and LTR enabling logic to share a same pci
device table.

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
(cherry picked from commit 47c7bfd31514e7b54a1f830f7707297aebbb8679)
---
 drivers/pci/quirks.c | 48 +++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 45 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 5c9f0bd71558..4f116ef32f70 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -6278,13 +6278,55 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_GLI, 0x9750, pci_mask_replay_timer_timeout
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_GLI, 0x9755, pci_mask_replay_timer_timeout);
 #endif
 /*
- * Device [8086:9a09]
+ * Device [8086:9a09], [8086:a0b0] and [8086:a0bc]
  * BIOS may not be able to access config space of devices under VMD domain, so
  * it relies on software to enable ASPM for links under VMD.
  */
+static const struct pci_device_id vmd_bridge_tbl[] = {
+	{ PCI_VDEVICE(INTEL, 0x9a09) },
+	{ PCI_VDEVICE(INTEL, 0xa0b0) },
+	{ PCI_VDEVICE(INTEL, 0xa0bc) },
+	{ }
+};
+
 static void pci_fixup_enable_aspm(struct pci_dev *pdev)
 {
+	if (!pci_match_id(vmd_bridge_tbl, pdev))
+		return;
+
 	pdev->dev_flags |= PCI_DEV_FLAGS_ENABLE_ASPM;
 }
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x9a09, pci_fixup_enable_aspm);
-DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0xa0b0, pci_fixup_enable_aspm);
+DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_VENDOR_ID_INTEL, PCI_ANY_ID,
+			       PCI_CLASS_BRIDGE_PCI, 8, pci_fixup_enable_aspm);
+
+static void pci_fixup_enable_vmd_nvme_ltr(struct pci_dev *pdev)
+{
+	struct pci_dev *parent;
+	int pos;
+	u16 val;
+
+	parent = pci_upstream_bridge(pdev);
+	if (!parent)
+		return;
+
+	if (!pci_match_id(vmd_bridge_tbl, parent))
+		return;
+
+	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_LTR);
+	if (!pos)
+		return;
+
+	pci_read_config_word(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, &val);
+	if (val)
+		return;
+
+	pci_read_config_word(pdev, pos + PCI_LTR_MAX_NOSNOOP_LAT, &val);
+	if (val)
+		return;
+
+	/* 3145728ns, i.e. 0x300000ns */
+	pci_write_config_word(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, 0x1003);
+	pci_write_config_word(pdev, pos + PCI_LTR_MAX_NOSNOOP_LAT, 0x1003);
+}
+DECLARE_PCI_FIXUP_CLASS_EARLY(PCI_ANY_ID, PCI_ANY_ID,
+			      PCI_CLASS_STORAGE_EXPRESS, 0, pci_fixup_enable_vmd_nvme_ltr);
-- 
2.43.0


[-- Attachment #4: 0001-UBUNTU-SAUCE-vmd-fixup-bridge-ASPM-by-driver-name-in.patch --]
[-- Type: text/x-patch, Size: 3013 bytes --]

From 255e345a6ab1c53f9607c41daa1eafac4bf83e0d Mon Sep 17 00:00:00 2001
From: You-Sheng Yang <vicamo.yang@canonical.com>
Date: Mon, 11 Apr 2022 17:24:34 +0800
Subject: [PATCH] UBUNTU: SAUCE: vmd: fixup bridge ASPM by driver name instead

BugLink: https://bugs.launchpad.net/bugs/1942160

Additional VMD bridge IDs needed for new Alder Lake platforms, but
actually there is no a complete list for them. Here we match bridge
devices if they're directly attached to a VMD controller instead.

Signed-off-by: You-Sheng Yang <vicamo.yang@canonical.com>
Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
(cherry picked from commit 154d48da2c57514e4b5dadc7b8c70a4edb550981)
---
 drivers/pci/quirks.c | 32 +++++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 4f116ef32f70..3e69d75242fd 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -6278,23 +6278,36 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_GLI, 0x9750, pci_mask_replay_timer_timeout
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_GLI, 0x9755, pci_mask_replay_timer_timeout);
 #endif
 /*
- * Device [8086:9a09], [8086:a0b0] and [8086:a0bc]
  * BIOS may not be able to access config space of devices under VMD domain, so
  * it relies on software to enable ASPM for links under VMD.
  */
-static const struct pci_device_id vmd_bridge_tbl[] = {
-	{ PCI_VDEVICE(INTEL, 0x9a09) },
-	{ PCI_VDEVICE(INTEL, 0xa0b0) },
-	{ PCI_VDEVICE(INTEL, 0xa0bc) },
-	{ }
-};
+static bool pci_fixup_is_vmd_bridge(struct pci_dev *pdev)
+{
+	struct pci_bus *bus = pdev->bus;
+	struct device *dev;
+	struct pci_driver *pdrv;
+
+	if (!pci_is_root_bus(bus))
+		return false;
+
+	dev = bus->bridge->parent;
+	if (dev == NULL)
+		return false;
+
+	pdrv = pci_dev_driver(to_pci_dev(dev));
+	if (pdrv == NULL || strcmp("vmd", pdrv->name))
+		return false;
+
+	return true;
+}
 
 static void pci_fixup_enable_aspm(struct pci_dev *pdev)
 {
-	if (!pci_match_id(vmd_bridge_tbl, pdev))
+	if (!pci_fixup_is_vmd_bridge(pdev))
 		return;
 
 	pdev->dev_flags |= PCI_DEV_FLAGS_ENABLE_ASPM;
+	pci_info(pdev, "enable ASPM for pci bridge behind vmd");
 }
 DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_VENDOR_ID_INTEL, PCI_ANY_ID,
 			       PCI_CLASS_BRIDGE_PCI, 8, pci_fixup_enable_aspm);
@@ -6309,7 +6322,7 @@ static void pci_fixup_enable_vmd_nvme_ltr(struct pci_dev *pdev)
 	if (!parent)
 		return;
 
-	if (!pci_match_id(vmd_bridge_tbl, parent))
+	if (!pci_fixup_is_vmd_bridge(parent))
 		return;
 
 	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_LTR);
@@ -6327,6 +6340,7 @@ static void pci_fixup_enable_vmd_nvme_ltr(struct pci_dev *pdev)
 	/* 3145728ns, i.e. 0x300000ns */
 	pci_write_config_word(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, 0x1003);
 	pci_write_config_word(pdev, pos + PCI_LTR_MAX_NOSNOOP_LAT, 0x1003);
+	pci_info(pdev, "enable LTR for nvme behind vmd");
 }
 DECLARE_PCI_FIXUP_CLASS_EARLY(PCI_ANY_ID, PCI_ANY_ID,
 			      PCI_CLASS_STORAGE_EXPRESS, 0, pci_fixup_enable_vmd_nvme_ltr);
-- 
2.43.0

Re: My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

[Bjorn Helgaas]

[+cc David, Nirmal, linux-pci]

On Thu, Dec 12, 2024 at 03:04:53PM -0800, Kenneth Crudup wrote:
> On 12/12/24 12:56, Bjorn Helgaas wrote:
> > On Wed, Dec 11, 2024 at 03:26:37PM -0800, Kenneth Crudup wrote:
> > > So we're on 6.13-rc2 and the patches are getting closer and
> > > closer, but they still need to be manually added.
> > > 
> > > The good news is now only (variants of) "PCI/ASPM: Enable LTR
> > > for endpoints behind VMD" and "PCI/ASPM: Enable ASPM for links
> > > under VMD domain" are needed.
> ...

> https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/lunar/commit/?id=47c7bfd31514e7b54a1f830f7707297aebbb8679

This is "UBUNTU: SAUCE: PCI/ASPM: Enable LTR for endpoints behind
VMD", which writes PCI_LTR_MAX_SNOOP_LAT and PCI_LTR_MAX_NOSNOOP_LAT
for PCI_CLASS_STORAGE_EXPRESS devices directly below VMDs with Device
IDs 0x9a09, 0xa0b0, or 0xa0bc.

This looks equivalent in spirit to upstream
https://git.kernel.org/linus/f492edb40b54 ("PCI: vmd: Add quirk to
configure PCIe ASPM and LTR"), which writes PCI_LTR_MAX_SNOOP_LAT and
PCI_LTR_MAX_NOSNOOP_LAT for any kind of device below VMDs tagged with
VMD_FEATS_CLIENT, which includes 0x467f, 0x4c3d, 0x7d0b, 0x9a0b,
0xa77f, 0xad0b, 0xb06f, 0xb60b.

However, IDs 0x9a09, 0xa0b0, and 0xa0bc are NOT tagged with
VMD_FEATS_CLIENT.  In fact, they're not included in vmd_ids[] at all,
so I'd be surprised if VMD worked for those devices unless BIOS set
up the VMD itself.

Maybe David or Nirmal can comment on this?

> https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/lunar/commit/?id=1a0102a08f206149d9abd56c2b28877c878b5526

This is "UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under VMD
domain", which adds "link->aspm_default = ASPM_STATE_ALL" for device
IDs 0x9a09 and 0xa0b0.

This looks like it should also be handled by upstream f492edb40b54
("PCI: vmd: Add quirk to configure PCIe ASPM and LTR") [1], which adds
"pci_enable_link_state(pdev, PCIE_LINK_STATE_ALL)".

But again, the Device IDs mentioned in the Ubuntu commit are NOT
included in the upstream VMD_FEATS_CLIENT list.

> https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/lunar/commit/?id=154d48da2c57514e4b5dadc7b8c70a4edb550981

This is "UBUNTU: SAUCE: vmd: fixup bridge ASPM by driver name
instead", which applies the quirk that writes PCI_LTR_MAX_SNOOP_LAT
and PCI_LTR_MAX_NOSNOOP_LAT for PCI_CLASS_STORAGE_EXPRESS devices
below any VMD claimed by the "vmd" driver, not just VMDs with Device
IDs 0x9a09, 0xa0b0, or 0xa0bc.

I think the only thing that's missing is that the upstream vmd_ids[]
needs to be updated with some new VMD Device IDs that are tagged with
VMD_FEATS_CLIENT.

I don't know what the vmd_ids[] strategy is, but Kenneth, you might
try an upstream patch like the one below.  If that resolves the
standby/low-power issues, maybe David or Nirmal can figure out the
"right" way to do this.

Bjorn

diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index 9d9596947350..4de7ff3bbf23 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -1109,6 +1109,12 @@ static const struct pci_device_id vmd_ids[] = {
                 .driver_data = VMD_FEATS_CLIENT,},
 	{PCI_VDEVICE(INTEL, 0xb06f),
                 .driver_data = VMD_FEATS_CLIENT,},
+	{PCI_VDEVICE(INTEL, 0x9a09),
+                .driver_data = VMD_FEATS_CLIENT,},
+	{PCI_VDEVICE(INTEL, 0xa0b0),
+                .driver_data = VMD_FEATS_CLIENT,},
+	{PCI_VDEVICE(INTEL, 0xa0bc),
+                .driver_data = VMD_FEATS_CLIENT,},
 	{0,}
 };
 MODULE_DEVICE_TABLE(pci, vmd_ids);

Re: My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

[Kenneth Crudup]

[-- Attachment #1: Type: text/plain, Size: 4787 bytes --]


I was really hopeful this would have handled it, but no joy.

I also tried dropping "PCI/ASPM: Enable LTR for endpoints behind VMD" 
and "PCI/ASPM: Enable ASPM for links under VMD domain" each separately 
on top of the below quirk patch to no avail.

The only thing that works is the aggregate patch I've added.

 > However, IDs 0x9a09, 0xa0b0, and 0xa0bc are NOT tagged with
 > VMD_FEATS_CLIENT.  In fact, they're not included in vmd_ids[] at all,
 > so I'd be surprised if VMD worked for those devices unless BIOS set
 > up the VMD itself.

Yeah, my BIOS does- not sure if you'd missed it, but I'd rewritten 
"Enable LTR for endpoints behind VMD" to print if the BIOS already does 
that for you, and sent the patch here (since I wasn't seeing the message 
printed when the fixup was being done and wanted to know why).

I'd REALLY like to get this into mainline, so if there's anything I can 
do to help, LMK.

Thanks,
-Kenny


On 12/13/24 08:43, Bjorn Helgaas wrote:
> [+cc David, Nirmal, linux-pci]
> 
> On Thu, Dec 12, 2024 at 03:04:53PM -0800, Kenneth Crudup wrote:
>> On 12/12/24 12:56, Bjorn Helgaas wrote:
>>> On Wed, Dec 11, 2024 at 03:26:37PM -0800, Kenneth Crudup wrote:
>>>> So we're on 6.13-rc2 and the patches are getting closer and
>>>> closer, but they still need to be manually added.
>>>>
>>>> The good news is now only (variants of) "PCI/ASPM: Enable LTR
>>>> for endpoints behind VMD" and "PCI/ASPM: Enable ASPM for links
>>>> under VMD domain" are needed.
>> ...
> 
>> https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/lunar/commit/?id=47c7bfd31514e7b54a1f830f7707297aebbb8679
> 
> This is "UBUNTU: SAUCE: PCI/ASPM: Enable LTR for endpoints behind
> VMD", which writes PCI_LTR_MAX_SNOOP_LAT and PCI_LTR_MAX_NOSNOOP_LAT
> for PCI_CLASS_STORAGE_EXPRESS devices directly below VMDs with Device
> IDs 0x9a09, 0xa0b0, or 0xa0bc.
> 
> This looks equivalent in spirit to upstream
> https://git.kernel.org/linus/f492edb40b54 ("PCI: vmd: Add quirk to
> configure PCIe ASPM and LTR"), which writes PCI_LTR_MAX_SNOOP_LAT and
> PCI_LTR_MAX_NOSNOOP_LAT for any kind of device below VMDs tagged with
> VMD_FEATS_CLIENT, which includes 0x467f, 0x4c3d, 0x7d0b, 0x9a0b,
> 0xa77f, 0xad0b, 0xb06f, 0xb60b.
> 
> However, IDs 0x9a09, 0xa0b0, and 0xa0bc are NOT tagged with
> VMD_FEATS_CLIENT.  In fact, they're not included in vmd_ids[] at all,
> so I'd be surprised if VMD worked for those devices unless BIOS set
> up the VMD itself.
> 
> Maybe David or Nirmal can comment on this?
> 
>> https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/lunar/commit/?id=1a0102a08f206149d9abd56c2b28877c878b5526
> 
> This is "UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under VMD
> domain", which adds "link->aspm_default = ASPM_STATE_ALL" for device
> IDs 0x9a09 and 0xa0b0.
> 
> This looks like it should also be handled by upstream f492edb40b54
> ("PCI: vmd: Add quirk to configure PCIe ASPM and LTR") [1], which adds
> "pci_enable_link_state(pdev, PCIE_LINK_STATE_ALL)".
> 
> But again, the Device IDs mentioned in the Ubuntu commit are NOT
> included in the upstream VMD_FEATS_CLIENT list.
> 
>> https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/lunar/commit/?id=154d48da2c57514e4b5dadc7b8c70a4edb550981
> 
> This is "UBUNTU: SAUCE: vmd: fixup bridge ASPM by driver name
> instead", which applies the quirk that writes PCI_LTR_MAX_SNOOP_LAT
> and PCI_LTR_MAX_NOSNOOP_LAT for PCI_CLASS_STORAGE_EXPRESS devices
> below any VMD claimed by the "vmd" driver, not just VMDs with Device
> IDs 0x9a09, 0xa0b0, or 0xa0bc.
> 
> I think the only thing that's missing is that the upstream vmd_ids[]
> needs to be updated with some new VMD Device IDs that are tagged with
> VMD_FEATS_CLIENT.
> 
> I don't know what the vmd_ids[] strategy is, but Kenneth, you might
> try an upstream patch like the one below.  If that resolves the
> standby/low-power issues, maybe David or Nirmal can figure out the
> "right" way to do this.
> 
> Bjorn
> 
> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index 9d9596947350..4de7ff3bbf23 100644
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c
> @@ -1109,6 +1109,12 @@ static const struct pci_device_id vmd_ids[] = {
>                   .driver_data = VMD_FEATS_CLIENT,},
>   	{PCI_VDEVICE(INTEL, 0xb06f),
>                   .driver_data = VMD_FEATS_CLIENT,},
> +	{PCI_VDEVICE(INTEL, 0x9a09),
> +                .driver_data = VMD_FEATS_CLIENT,},
> +	{PCI_VDEVICE(INTEL, 0xa0b0),
> +                .driver_data = VMD_FEATS_CLIENT,},
> +	{PCI_VDEVICE(INTEL, 0xa0bc),
> +                .driver_data = VMD_FEATS_CLIENT,},
>   	{0,}
>   };
>   MODULE_DEVICE_TABLE(pci, vmd_ids);
> 

-- 
Kenneth R. Crudup / Sr. SW Engineer, Scott County Consulting, Orange 
County CA

[-- Attachment #2: aspm-vmd-patch --]
[-- Type: text/plain, Size: 3289 bytes --]

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 28567d457613..6cec8ed1a726 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -846,7 +846,8 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	}
 
 	/* Save default state */
-	link->aspm_default = link->aspm_enabled;
+	link->aspm_default = parent->dev_flags & PCI_DEV_FLAGS_ENABLE_ASPM ?
+			     PCIE_LINK_STATE_ASPM_ALL : link->aspm_enabled;
 
 	/* Setup initial capable state. Will be updated later */
 	link->aspm_capable = link->aspm_support;
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 76f4df75b08a..c5145e74df73 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -6301,3 +6301,70 @@ static void pci_mask_replay_timer_timeout(struct pci_dev *pdev)
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_GLI, 0x9750, pci_mask_replay_timer_timeout);
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_GLI, 0x9755, pci_mask_replay_timer_timeout);
 #endif
+/*
+ * BIOS may not be able to access config space of devices under VMD domain, so
+ * it relies on software to enable ASPM for links under VMD.
+ */
+static bool pci_fixup_is_vmd_bridge(struct pci_dev *pdev)
+{
+	struct pci_bus *bus = pdev->bus;
+	struct device *dev;
+	struct pci_driver *pdrv;
+
+	if (!pci_is_root_bus(bus))
+		return false;
+
+	dev = bus->bridge->parent;
+	if (dev == NULL)
+		return false;
+
+	pdrv = pci_dev_driver(to_pci_dev(dev));
+	if (pdrv == NULL || strcmp("vmd", pdrv->name))
+		return false;
+
+	return true;
+}
+
+static void pci_fixup_enable_aspm(struct pci_dev *pdev)
+{
+	if (!pci_fixup_is_vmd_bridge(pdev))
+		return;
+
+	pdev->dev_flags |= PCI_DEV_FLAGS_ENABLE_ASPM;
+	pci_info(pdev, "enable ASPM for pci bridge behind vmd");
+}
+DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_VENDOR_ID_INTEL, PCI_ANY_ID,
+			       PCI_CLASS_BRIDGE_PCI, 8, pci_fixup_enable_aspm);
+
+static void pci_fixup_enable_vmd_nvme_ltr(struct pci_dev *pdev)
+{
+	struct pci_dev *parent;
+	int pos;
+	u16 val;
+
+	parent = pci_upstream_bridge(pdev);
+	if (!parent)
+		return;
+
+	if (!pci_fixup_is_vmd_bridge(parent))
+		return;
+
+	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_LTR);
+	if (!pos)
+		return;
+
+	pci_read_config_word(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, &val);
+	if (val)
+		return;
+
+	pci_read_config_word(pdev, pos + PCI_LTR_MAX_NOSNOOP_LAT, &val);
+	if (val)
+		return;
+
+	/* 3145728ns, i.e. 0x300000ns */
+	pci_write_config_word(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, 0x1003);
+	pci_write_config_word(pdev, pos + PCI_LTR_MAX_NOSNOOP_LAT, 0x1003);
+	pci_info(pdev, "enable LTR for nvme behind vmd");
+}
+DECLARE_PCI_FIXUP_CLASS_EARLY(PCI_ANY_ID, PCI_ANY_ID,
+			      PCI_CLASS_STORAGE_EXPRESS, 0, pci_fixup_enable_vmd_nvme_ltr);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index db9b47ce3eef..9bd8234f1d39 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -245,6 +245,8 @@ enum pci_dev_flags {
 	PCI_DEV_FLAGS_NO_RELAXED_ORDERING = (__force pci_dev_flags_t) (1 << 11),
 	/* Device does honor MSI masking despite saying otherwise */
 	PCI_DEV_FLAGS_HAS_MSI_MASKING = (__force pci_dev_flags_t) (1 << 12),
+	/* Enable ASPM regardless of how LnkCtl is programmed */
+	PCI_DEV_FLAGS_ENABLE_ASPM = (__force pci_dev_flags_t) (1 << 13),
 };
 
 enum pci_irq_reroute_variant {

Re: My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

[Kenneth Crudup]

Turns out that my current VMD is already listed as "VMD_FEATS_CLIENT" 
after all:

----
         {PCI_VDEVICE(INTEL, 0x467f),
                 .driver_data = VMD_FEATS_CLIENT,},
----
0000:00:0e.0 0104: 8086:467f
         Subsystem: 1028:0af3
         Flags: bus master, fast devsel, latency 0, IOMMU group 9
         Memory at 603c000000 (64-bit, non-prefetchable) [size=32M]
         Memory at 72000000 (32-bit, non-prefetchable) [size=32M]
         Memory at 6040100000 (64-bit, non-prefetchable) [size=1M]
         Capabilities: [80] MSI-X: Enable+ Count=19 Masked-
         Capabilities: [90] Express Root Complex Integrated Endpoint, MSI 00
         Capabilities: [e0] Power Management version 3
         Kernel driver in use: vmd
----

I'm going to compare the commits you'd pointed out in master to what the 
working (Ubuntu) patches do and figure out where the disconnect is.

-K

On 12/13/24 11:48, Kenneth Crudup wrote:
> 
> I was really hopeful this would have handled it, but no joy.
> 
> I also tried dropping "PCI/ASPM: Enable LTR for endpoints behind VMD" 
> and "PCI/ASPM: Enable ASPM for links under VMD domain" each separately 
> on top of the below quirk patch to no avail.
> 
> The only thing that works is the aggregate patch I've added.
> 
>  > However, IDs 0x9a09, 0xa0b0, and 0xa0bc are NOT tagged with
>  > VMD_FEATS_CLIENT.  In fact, they're not included in vmd_ids[] at all,
>  > so I'd be surprised if VMD worked for those devices unless BIOS set
>  > up the VMD itself.
> 
> Yeah, my BIOS does- not sure if you'd missed it, but I'd rewritten 
> "Enable LTR for endpoints behind VMD" to print if the BIOS already does 
> that for you, and sent the patch here (since I wasn't seeing the message 
> printed when the fixup was being done and wanted to know why).
> 
> I'd REALLY like to get this into mainline, so if there's anything I can 
> do to help, LMK.
> 
> Thanks,
> -Kenny
> 
> 
> On 12/13/24 08:43, Bjorn Helgaas wrote:
>> [+cc David, Nirmal, linux-pci]
>>
>> On Thu, Dec 12, 2024 at 03:04:53PM -0800, Kenneth Crudup wrote:
>>> On 12/12/24 12:56, Bjorn Helgaas wrote:
>>>> On Wed, Dec 11, 2024 at 03:26:37PM -0800, Kenneth Crudup wrote:
>>>>> So we're on 6.13-rc2 and the patches are getting closer and
>>>>> closer, but they still need to be manually added.
>>>>>
>>>>> The good news is now only (variants of) "PCI/ASPM: Enable LTR
>>>>> for endpoints behind VMD" and "PCI/ASPM: Enable ASPM for links
>>>>> under VMD domain" are needed.
>>> ...
>>
>>> https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/ 
>>> lunar/commit/?id=47c7bfd31514e7b54a1f830f7707297aebbb8679
>>
>> This is "UBUNTU: SAUCE: PCI/ASPM: Enable LTR for endpoints behind
>> VMD", which writes PCI_LTR_MAX_SNOOP_LAT and PCI_LTR_MAX_NOSNOOP_LAT
>> for PCI_CLASS_STORAGE_EXPRESS devices directly below VMDs with Device
>> IDs 0x9a09, 0xa0b0, or 0xa0bc.
>>
>> This looks equivalent in spirit to upstream
>> https://git.kernel.org/linus/f492edb40b54 ("PCI: vmd: Add quirk to
>> configure PCIe ASPM and LTR"), which writes PCI_LTR_MAX_SNOOP_LAT and
>> PCI_LTR_MAX_NOSNOOP_LAT for any kind of device below VMDs tagged with
>> VMD_FEATS_CLIENT, which includes 0x467f, 0x4c3d, 0x7d0b, 0x9a0b,
>> 0xa77f, 0xad0b, 0xb06f, 0xb60b.
>>
>> However, IDs 0x9a09, 0xa0b0, and 0xa0bc are NOT tagged with
>> VMD_FEATS_CLIENT.  In fact, they're not included in vmd_ids[] at all,
>> so I'd be surprised if VMD worked for those devices unless BIOS set
>> up the VMD itself.
>>
>> Maybe David or Nirmal can comment on this?
>>
>>> https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/ 
>>> lunar/commit/?id=1a0102a08f206149d9abd56c2b28877c878b5526
>>
>> This is "UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under VMD
>> domain", which adds "link->aspm_default = ASPM_STATE_ALL" for device
>> IDs 0x9a09 and 0xa0b0.
>>
>> This looks like it should also be handled by upstream f492edb40b54
>> ("PCI: vmd: Add quirk to configure PCIe ASPM and LTR") [1], which adds
>> "pci_enable_link_state(pdev, PCIE_LINK_STATE_ALL)".
>>
>> But again, the Device IDs mentioned in the Ubuntu commit are NOT
>> included in the upstream VMD_FEATS_CLIENT list.
>>
>>> https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/ 
>>> lunar/commit/?id=154d48da2c57514e4b5dadc7b8c70a4edb550981
>>
>> This is "UBUNTU: SAUCE: vmd: fixup bridge ASPM by driver name
>> instead", which applies the quirk that writes PCI_LTR_MAX_SNOOP_LAT
>> and PCI_LTR_MAX_NOSNOOP_LAT for PCI_CLASS_STORAGE_EXPRESS devices
>> below any VMD claimed by the "vmd" driver, not just VMDs with Device
>> IDs 0x9a09, 0xa0b0, or 0xa0bc.
>>
>> I think the only thing that's missing is that the upstream vmd_ids[]
>> needs to be updated with some new VMD Device IDs that are tagged with
>> VMD_FEATS_CLIENT.
>>
>> I don't know what the vmd_ids[] strategy is, but Kenneth, you might
>> try an upstream patch like the one below.  If that resolves the
>> standby/low-power issues, maybe David or Nirmal can figure out the
>> "right" way to do this.
>>
>> Bjorn
>>
>> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
>> index 9d9596947350..4de7ff3bbf23 100644
>> --- a/drivers/pci/controller/vmd.c
>> +++ b/drivers/pci/controller/vmd.c
>> @@ -1109,6 +1109,12 @@ static const struct pci_device_id vmd_ids[] = {
>>                   .driver_data = VMD_FEATS_CLIENT,},
>>       {PCI_VDEVICE(INTEL, 0xb06f),
>>                   .driver_data = VMD_FEATS_CLIENT,},
>> +    {PCI_VDEVICE(INTEL, 0x9a09),
>> +                .driver_data = VMD_FEATS_CLIENT,},
>> +    {PCI_VDEVICE(INTEL, 0xa0b0),
>> +                .driver_data = VMD_FEATS_CLIENT,},
>> +    {PCI_VDEVICE(INTEL, 0xa0bc),
>> +                .driver_data = VMD_FEATS_CLIENT,},
>>       {0,}
>>   };
>>   MODULE_DEVICE_TABLE(pci, vmd_ids);
>>
> 

-- 
Kenneth R. Crudup / Sr. SW Engineer, Scott County Consulting, Orange 
County CA

Re: My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

[Kenneth Crudup]

[-- Attachment #1: Type: text/plain, Size: 7538 bytes --]


OK, it looks like the effective change (that's not already contained in 
the LTR SNOOP patches already in Linus' master (et al.)) comes from this 
line from the Ubuntu commit 1a0102a0 ("UBUNTU: SAUCE: PCI/ASPM: Enable 
ASPM for links under VMD domain"):

----
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 00143f5fb83a..d2ff44e7fbb1 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -688,7 +688,8 @@ static void pcie_aspm_cap_init(struct 
pcie_link_state *link, int blacklist)
         aspm_l1ss_init(link);

         /* Save default state */
-       link->aspm_default = link->aspm_enabled;
+       link->aspm_default = parent->dev_flags & PCI_DEV_FLAGS_ENABLE_ASPM ?
+                            ASPM_STATE_ALL : link->aspm_enabled;
----

Where "PCI_DEV_FLAGS_ENABLE_ASPM" (a new flag) is set if the parent 
bridge is a VMD (determined via Ubuntu commit 154d48da2 ("UBUNTU: SAUCE: 
vmd: fixup bridge ASPM by driver name instead").

So, as we've both noticed, most of the three patches I'd pointed out 
aren't required as the LTR SNOOP values are either fixed up, or 
previously set in the BIOS.

So I've folded the new commits into the attached patch (against Linus' 
master), which now gives me full power-saving.

-K


On 12/13/24 12:27, Kenneth Crudup wrote:
> 
> Turns out that my current VMD is already listed as "VMD_FEATS_CLIENT" 
> after all:
> 
> ----
>          {PCI_VDEVICE(INTEL, 0x467f),
>                  .driver_data = VMD_FEATS_CLIENT,},
> ----
> 0000:00:0e.0 0104: 8086:467f
>          Subsystem: 1028:0af3
>          Flags: bus master, fast devsel, latency 0, IOMMU group 9
>          Memory at 603c000000 (64-bit, non-prefetchable) [size=32M]
>          Memory at 72000000 (32-bit, non-prefetchable) [size=32M]
>          Memory at 6040100000 (64-bit, non-prefetchable) [size=1M]
>          Capabilities: [80] MSI-X: Enable+ Count=19 Masked-
>          Capabilities: [90] Express Root Complex Integrated Endpoint, 
> MSI 00
>          Capabilities: [e0] Power Management version 3
>          Kernel driver in use: vmd
> ----
> 
> I'm going to compare the commits you'd pointed out in master to what the 
> working (Ubuntu) patches do and figure out where the disconnect is.
> 
> -K
> 
> On 12/13/24 11:48, Kenneth Crudup wrote:
>>
>> I was really hopeful this would have handled it, but no joy.
>>
>> I also tried dropping "PCI/ASPM: Enable LTR for endpoints behind VMD" 
>> and "PCI/ASPM: Enable ASPM for links under VMD domain" each separately 
>> on top of the below quirk patch to no avail.
>>
>> The only thing that works is the aggregate patch I've added.
>>
>>  > However, IDs 0x9a09, 0xa0b0, and 0xa0bc are NOT tagged with
>>  > VMD_FEATS_CLIENT.  In fact, they're not included in vmd_ids[] at all,
>>  > so I'd be surprised if VMD worked for those devices unless BIOS set
>>  > up the VMD itself.
>>
>> Yeah, my BIOS does- not sure if you'd missed it, but I'd rewritten 
>> "Enable LTR for endpoints behind VMD" to print if the BIOS already 
>> does that for you, and sent the patch here (since I wasn't seeing the 
>> message printed when the fixup was being done and wanted to know why).
>>
>> I'd REALLY like to get this into mainline, so if there's anything I 
>> can do to help, LMK.
>>
>> Thanks,
>> -Kenny
>>
>>
>> On 12/13/24 08:43, Bjorn Helgaas wrote:
>>> [+cc David, Nirmal, linux-pci]
>>>
>>> On Thu, Dec 12, 2024 at 03:04:53PM -0800, Kenneth Crudup wrote:
>>>> On 12/12/24 12:56, Bjorn Helgaas wrote:
>>>>> On Wed, Dec 11, 2024 at 03:26:37PM -0800, Kenneth Crudup wrote:
>>>>>> So we're on 6.13-rc2 and the patches are getting closer and
>>>>>> closer, but they still need to be manually added.
>>>>>>
>>>>>> The good news is now only (variants of) "PCI/ASPM: Enable LTR
>>>>>> for endpoints behind VMD" and "PCI/ASPM: Enable ASPM for links
>>>>>> under VMD domain" are needed.
>>>> ...
>>>
>>>> https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/ 
>>>> lunar/commit/?id=47c7bfd31514e7b54a1f830f7707297aebbb8679
>>>
>>> This is "UBUNTU: SAUCE: PCI/ASPM: Enable LTR for endpoints behind
>>> VMD", which writes PCI_LTR_MAX_SNOOP_LAT and PCI_LTR_MAX_NOSNOOP_LAT
>>> for PCI_CLASS_STORAGE_EXPRESS devices directly below VMDs with Device
>>> IDs 0x9a09, 0xa0b0, or 0xa0bc.
>>>
>>> This looks equivalent in spirit to upstream
>>> https://git.kernel.org/linus/f492edb40b54 ("PCI: vmd: Add quirk to
>>> configure PCIe ASPM and LTR"), which writes PCI_LTR_MAX_SNOOP_LAT and
>>> PCI_LTR_MAX_NOSNOOP_LAT for any kind of device below VMDs tagged with
>>> VMD_FEATS_CLIENT, which includes 0x467f, 0x4c3d, 0x7d0b, 0x9a0b,
>>> 0xa77f, 0xad0b, 0xb06f, 0xb60b.
>>>
>>> However, IDs 0x9a09, 0xa0b0, and 0xa0bc are NOT tagged with
>>> VMD_FEATS_CLIENT.  In fact, they're not included in vmd_ids[] at all,
>>> so I'd be surprised if VMD worked for those devices unless BIOS set
>>> up the VMD itself.
>>>
>>> Maybe David or Nirmal can comment on this?
>>>
>>>> https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/ 
>>>> lunar/commit/?id=1a0102a08f206149d9abd56c2b28877c878b5526
>>>
>>> This is "UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under VMD
>>> domain", which adds "link->aspm_default = ASPM_STATE_ALL" for device
>>> IDs 0x9a09 and 0xa0b0.
>>>
>>> This looks like it should also be handled by upstream f492edb40b54
>>> ("PCI: vmd: Add quirk to configure PCIe ASPM and LTR") [1], which adds
>>> "pci_enable_link_state(pdev, PCIE_LINK_STATE_ALL)".
>>>
>>> But again, the Device IDs mentioned in the Ubuntu commit are NOT
>>> included in the upstream VMD_FEATS_CLIENT list.
>>>
>>>> https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/ 
>>>> lunar/commit/?id=154d48da2c57514e4b5dadc7b8c70a4edb550981
>>>
>>> This is "UBUNTU: SAUCE: vmd: fixup bridge ASPM by driver name
>>> instead", which applies the quirk that writes PCI_LTR_MAX_SNOOP_LAT
>>> and PCI_LTR_MAX_NOSNOOP_LAT for PCI_CLASS_STORAGE_EXPRESS devices
>>> below any VMD claimed by the "vmd" driver, not just VMDs with Device
>>> IDs 0x9a09, 0xa0b0, or 0xa0bc.
>>>
>>> I think the only thing that's missing is that the upstream vmd_ids[]
>>> needs to be updated with some new VMD Device IDs that are tagged with
>>> VMD_FEATS_CLIENT.
>>>
>>> I don't know what the vmd_ids[] strategy is, but Kenneth, you might
>>> try an upstream patch like the one below.  If that resolves the
>>> standby/low-power issues, maybe David or Nirmal can figure out the
>>> "right" way to do this.
>>>
>>> Bjorn
>>>
>>> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
>>> index 9d9596947350..4de7ff3bbf23 100644
>>> --- a/drivers/pci/controller/vmd.c
>>> +++ b/drivers/pci/controller/vmd.c
>>> @@ -1109,6 +1109,12 @@ static const struct pci_device_id vmd_ids[] = {
>>>                   .driver_data = VMD_FEATS_CLIENT,},
>>>       {PCI_VDEVICE(INTEL, 0xb06f),
>>>                   .driver_data = VMD_FEATS_CLIENT,},
>>> +    {PCI_VDEVICE(INTEL, 0x9a09),
>>> +                .driver_data = VMD_FEATS_CLIENT,},
>>> +    {PCI_VDEVICE(INTEL, 0xa0b0),
>>> +                .driver_data = VMD_FEATS_CLIENT,},
>>> +    {PCI_VDEVICE(INTEL, 0xa0bc),
>>> +                .driver_data = VMD_FEATS_CLIENT,},
>>>       {0,}
>>>   };
>>>   MODULE_DEVICE_TABLE(pci, vmd_ids);
>>>
>>
> 

-- 
Kenneth R. Crudup / Sr. SW Engineer, Scott County Consulting, Orange 
County CA

[-- Attachment #2: aspm-enable-take-2 --]
[-- Type: text/plain, Size: 1557 bytes --]

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 28567d457613..a5df6230cf3c 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -768,6 +768,31 @@ static void aspm_l1ss_init(struct pcie_link_state *link)
 		aspm_calc_l12_info(link, parent_l1ss_cap, child_l1ss_cap);
 }
 
+/*
+ * BIOS may not be able to access config space of devices under VMD domain, so
+ * it relies on software to enable ASPM for links under VMD.
+ */
+static bool pci_fixup_vmd_bridge_enable_aspm(struct pci_dev *pdev)
+{
+       struct pci_bus *bus = pdev->bus;
+       struct device *dev;
+       struct pci_driver *pdrv;
+
+       if (!pci_is_root_bus(bus))
+               return false;
+
+       dev = bus->bridge->parent;
+       if (dev == NULL)
+               return false;
+
+       pdrv = pci_dev_driver(to_pci_dev(dev));
+       if (pdrv == NULL || strcmp("vmd", pdrv->name))
+               return false;
+
+       pci_info(pdev, "enable ASPM for pci bridge behind vmd");
+       return true;
+}
+
 static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 {
 	struct pci_dev *child = link->downstream, *parent = link->pdev;
@@ -846,7 +871,8 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	}
 
 	/* Save default state */
-	link->aspm_default = link->aspm_enabled;
+	link->aspm_default = pci_fixup_vmd_bridge_enable_aspm(parent) ?
+		PCIE_LINK_STATE_ASPM_ALL : link->aspm_enabled;
 
 	/* Setup initial capable state. Will be updated later */
 	link->aspm_capable = link->aspm_support;

Re: My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

[Kenneth Crudup]

Ah, that sweet power savings! :)

----
C1E%    C10%    CPU%c7  PkgTmp  GFX%rc6 GFXMHz  GFXAMHz CPUGFX% Pkg%pc8 
Pk%pc10 CPU%LPI SYS%LPI PkgWatt CorWatt GFXWatt
0.65    93.77   36.12   49      0.00    1033    1050    2.93    7.53 
21.66   21.70   0.00    3.28    1.34    0.02
0.09    99.23   41.68   48      0.00    1033    1050    0.91    11.98 
10.22   10.24   0.00    1.95    0.22    0.01
0.09    99.17   41.41   49      0.00    1033    1050    0.25    29.83 
39.18   39.26   11.38   1.10    0.34    0.00
0.02    99.78   42.40   48      0.00    1033    1050    0.20    31.63 
48.31   48.40   42.58   0.55    0.11    0.00
0.06    99.35   41.67   46      0.00    1033    1050    0.21    23.86 
51.95   52.04   46.83   0.80    0.30    0.00
C1E%    C10%    CPU%c7  PkgTmp  GFX%rc6 GFXMHz  GFXAMHz CPUGFX% Pkg%pc8 
Pk%pc10 CPU%LPI SYS%LPI PkgWatt CorWatt GFXWatt
0.04    99.71   42.38   46      0.00    1033    1050    0.20    24.74 
58.05   58.15   51.85   0.44    0.11    0.00
0.07    99.41   41.77   46      0.00    1033    1050    0.17    25.20 
52.57   52.65   46.99   0.73    0.27    0.00
0.03    99.76   42.39   45      0.00    1033    1050    0.18    26.67 
56.59   56.68   50.16   0.46    0.11    0.00
0.06    99.24   41.54   45      0.00    1033    1050    0.24    23.89 
46.83   46.90   41.40   0.91    0.32    0.00
0.03    99.75   42.42   45      0.00    1033    1050    0.20    22.65 
59.64   59.73   52.87   0.46    0.10    0.00
----

-K

On 12/13/24 14:26, Kenneth Crudup wrote:
> 
> OK, it looks like the effective change (that's not already contained in 
> the LTR SNOOP patches already in Linus' master (et al.)) comes from this 
> line from the Ubuntu commit 1a0102a0 ("UBUNTU: SAUCE: PCI/ASPM: Enable 
> ASPM for links under VMD domain"):
> 
> ----
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 00143f5fb83a..d2ff44e7fbb1 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -688,7 +688,8 @@ static void pcie_aspm_cap_init(struct 
> pcie_link_state *link, int blacklist)
>          aspm_l1ss_init(link);
> 
>          /* Save default state */
> -       link->aspm_default = link->aspm_enabled;
> +       link->aspm_default = parent->dev_flags & 
> PCI_DEV_FLAGS_ENABLE_ASPM ?
> +                            ASPM_STATE_ALL : link->aspm_enabled;
> ----
> 
> Where "PCI_DEV_FLAGS_ENABLE_ASPM" (a new flag) is set if the parent 
> bridge is a VMD (determined via Ubuntu commit 154d48da2 ("UBUNTU: SAUCE: 
> vmd: fixup bridge ASPM by driver name instead").
> 
> So, as we've both noticed, most of the three patches I'd pointed out 
> aren't required as the LTR SNOOP values are either fixed up, or 
> previously set in the BIOS.
> 
> So I've folded the new commits into the attached patch (against Linus' 
> master), which now gives me full power-saving.
> 
> -K
> 
> 
> On 12/13/24 12:27, Kenneth Crudup wrote:
>>
>> Turns out that my current VMD is already listed as "VMD_FEATS_CLIENT" 
>> after all:
>>
>> ----
>>          {PCI_VDEVICE(INTEL, 0x467f),
>>                  .driver_data = VMD_FEATS_CLIENT,},
>> ----
>> 0000:00:0e.0 0104: 8086:467f
>>          Subsystem: 1028:0af3
>>          Flags: bus master, fast devsel, latency 0, IOMMU group 9
>>          Memory at 603c000000 (64-bit, non-prefetchable) [size=32M]
>>          Memory at 72000000 (32-bit, non-prefetchable) [size=32M]
>>          Memory at 6040100000 (64-bit, non-prefetchable) [size=1M]
>>          Capabilities: [80] MSI-X: Enable+ Count=19 Masked-
>>          Capabilities: [90] Express Root Complex Integrated Endpoint, 
>> MSI 00
>>          Capabilities: [e0] Power Management version 3
>>          Kernel driver in use: vmd
>> ----
>>
>> I'm going to compare the commits you'd pointed out in master to what 
>> the working (Ubuntu) patches do and figure out where the disconnect is.
>>
>> -K
>>
>> On 12/13/24 11:48, Kenneth Crudup wrote:
>>>
>>> I was really hopeful this would have handled it, but no joy.
>>>
>>> I also tried dropping "PCI/ASPM: Enable LTR for endpoints behind VMD" 
>>> and "PCI/ASPM: Enable ASPM for links under VMD domain" each 
>>> separately on top of the below quirk patch to no avail.
>>>
>>> The only thing that works is the aggregate patch I've added.
>>>
>>>  > However, IDs 0x9a09, 0xa0b0, and 0xa0bc are NOT tagged with
>>>  > VMD_FEATS_CLIENT.  In fact, they're not included in vmd_ids[] at all,
>>>  > so I'd be surprised if VMD worked for those devices unless BIOS set
>>>  > up the VMD itself.
>>>
>>> Yeah, my BIOS does- not sure if you'd missed it, but I'd rewritten 
>>> "Enable LTR for endpoints behind VMD" to print if the BIOS already 
>>> does that for you, and sent the patch here (since I wasn't seeing the 
>>> message printed when the fixup was being done and wanted to know why).
>>>
>>> I'd REALLY like to get this into mainline, so if there's anything I 
>>> can do to help, LMK.
>>>
>>> Thanks,
>>> -Kenny
>>>
>>>
>>> On 12/13/24 08:43, Bjorn Helgaas wrote:
>>>> [+cc David, Nirmal, linux-pci]
>>>>
>>>> On Thu, Dec 12, 2024 at 03:04:53PM -0800, Kenneth Crudup wrote:
>>>>> On 12/12/24 12:56, Bjorn Helgaas wrote:
>>>>>> On Wed, Dec 11, 2024 at 03:26:37PM -0800, Kenneth Crudup wrote:
>>>>>>> So we're on 6.13-rc2 and the patches are getting closer and
>>>>>>> closer, but they still need to be manually added.
>>>>>>>
>>>>>>> The good news is now only (variants of) "PCI/ASPM: Enable LTR
>>>>>>> for endpoints behind VMD" and "PCI/ASPM: Enable ASPM for links
>>>>>>> under VMD domain" are needed.
>>>>> ...
>>>>
>>>>> https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/ 
>>>>> lunar/commit/?id=47c7bfd31514e7b54a1f830f7707297aebbb8679
>>>>
>>>> This is "UBUNTU: SAUCE: PCI/ASPM: Enable LTR for endpoints behind
>>>> VMD", which writes PCI_LTR_MAX_SNOOP_LAT and PCI_LTR_MAX_NOSNOOP_LAT
>>>> for PCI_CLASS_STORAGE_EXPRESS devices directly below VMDs with Device
>>>> IDs 0x9a09, 0xa0b0, or 0xa0bc.
>>>>
>>>> This looks equivalent in spirit to upstream
>>>> https://git.kernel.org/linus/f492edb40b54 ("PCI: vmd: Add quirk to
>>>> configure PCIe ASPM and LTR"), which writes PCI_LTR_MAX_SNOOP_LAT and
>>>> PCI_LTR_MAX_NOSNOOP_LAT for any kind of device below VMDs tagged with
>>>> VMD_FEATS_CLIENT, which includes 0x467f, 0x4c3d, 0x7d0b, 0x9a0b,
>>>> 0xa77f, 0xad0b, 0xb06f, 0xb60b.
>>>>
>>>> However, IDs 0x9a09, 0xa0b0, and 0xa0bc are NOT tagged with
>>>> VMD_FEATS_CLIENT.  In fact, they're not included in vmd_ids[] at all,
>>>> so I'd be surprised if VMD worked for those devices unless BIOS set
>>>> up the VMD itself.
>>>>
>>>> Maybe David or Nirmal can comment on this?
>>>>
>>>>> https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/ 
>>>>> lunar/commit/?id=1a0102a08f206149d9abd56c2b28877c878b5526
>>>>
>>>> This is "UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under VMD
>>>> domain", which adds "link->aspm_default = ASPM_STATE_ALL" for device
>>>> IDs 0x9a09 and 0xa0b0.
>>>>
>>>> This looks like it should also be handled by upstream f492edb40b54
>>>> ("PCI: vmd: Add quirk to configure PCIe ASPM and LTR") [1], which adds
>>>> "pci_enable_link_state(pdev, PCIE_LINK_STATE_ALL)".
>>>>
>>>> But again, the Device IDs mentioned in the Ubuntu commit are NOT
>>>> included in the upstream VMD_FEATS_CLIENT list.
>>>>
>>>>> https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/ 
>>>>> lunar/commit/?id=154d48da2c57514e4b5dadc7b8c70a4edb550981
>>>>
>>>> This is "UBUNTU: SAUCE: vmd: fixup bridge ASPM by driver name
>>>> instead", which applies the quirk that writes PCI_LTR_MAX_SNOOP_LAT
>>>> and PCI_LTR_MAX_NOSNOOP_LAT for PCI_CLASS_STORAGE_EXPRESS devices
>>>> below any VMD claimed by the "vmd" driver, not just VMDs with Device
>>>> IDs 0x9a09, 0xa0b0, or 0xa0bc.
>>>>
>>>> I think the only thing that's missing is that the upstream vmd_ids[]
>>>> needs to be updated with some new VMD Device IDs that are tagged with
>>>> VMD_FEATS_CLIENT.
>>>>
>>>> I don't know what the vmd_ids[] strategy is, but Kenneth, you might
>>>> try an upstream patch like the one below.  If that resolves the
>>>> standby/low-power issues, maybe David or Nirmal can figure out the
>>>> "right" way to do this.
>>>>
>>>> Bjorn
>>>>
>>>> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/ 
>>>> vmd.c
>>>> index 9d9596947350..4de7ff3bbf23 100644
>>>> --- a/drivers/pci/controller/vmd.c
>>>> +++ b/drivers/pci/controller/vmd.c
>>>> @@ -1109,6 +1109,12 @@ static const struct pci_device_id vmd_ids[] = {
>>>>                   .driver_data = VMD_FEATS_CLIENT,},
>>>>       {PCI_VDEVICE(INTEL, 0xb06f),
>>>>                   .driver_data = VMD_FEATS_CLIENT,},
>>>> +    {PCI_VDEVICE(INTEL, 0x9a09),
>>>> +                .driver_data = VMD_FEATS_CLIENT,},
>>>> +    {PCI_VDEVICE(INTEL, 0xa0b0),
>>>> +                .driver_data = VMD_FEATS_CLIENT,},
>>>> +    {PCI_VDEVICE(INTEL, 0xa0bc),
>>>> +                .driver_data = VMD_FEATS_CLIENT,},
>>>>       {0,}
>>>>   };
>>>>   MODULE_DEVICE_TABLE(pci, vmd_ids);
>>>>
>>>
>>
> 

-- 
Kenneth R. Crudup / Sr. SW Engineer, Scott County Consulting, Orange 
County CA

Re: My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

[Bjorn Helgaas]

[cc->to: David, Nirmal]

On Fri, Dec 13, 2024 at 02:26:37PM -0800, Kenneth Crudup wrote:
> OK, it looks like the effective change (that's not already contained in the
> LTR SNOOP patches already in Linus' master (et al.)) comes from this line
> from the Ubuntu commit 1a0102a0 ("UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for
> links under VMD domain"):
> 
> ----
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 00143f5fb83a..d2ff44e7fbb1 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -688,7 +688,8 @@ static void pcie_aspm_cap_init(struct pcie_link_state
> *link, int blacklist)
>         aspm_l1ss_init(link);
> 
>         /* Save default state */
> -       link->aspm_default = link->aspm_enabled;
> +       link->aspm_default = parent->dev_flags & PCI_DEV_FLAGS_ENABLE_ASPM ?
> +                            ASPM_STATE_ALL : link->aspm_enabled;

So I thought the "pci_enable_link_state(pdev, PCIE_LINK_STATE_ALL)" in
f492edb40b54 would effectively do the same thing:

> > > > > https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/
> > > > > lunar/commit/?id=1a0102a08f206149d9abd56c2b28877c878b5526
> > > > 
> > > > This is "UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under VMD
> > > > domain", which adds "link->aspm_default = ASPM_STATE_ALL" for device
> > > > IDs 0x9a09 and 0xa0b0.
> > > > 
> > > > This looks like it should also be handled by upstream f492edb40b54
> > > > ("PCI: vmd: Add quirk to configure PCIe ASPM and LTR") [1], which adds
> > > > "pci_enable_link_state(pdev, PCIE_LINK_STATE_ALL)".

But I guess it doesn't actually work.  I'm hoping David or Nirmal can
figure out why it doesn't because it seems obvious that it's the
intent.

Thanks a lot for all your work to narrow it down to this!

Bjorn

Re: My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

[David E. Box]

Hi Kenneth,

On Fri, 2024-12-13 at 17:02 -0600, Bjorn Helgaas wrote:
> [cc->to: David, Nirmal]
> 
> On Fri, Dec 13, 2024 at 02:26:37PM -0800, Kenneth Crudup wrote:
> > OK, it looks like the effective change (that's not already contained in the
> > LTR SNOOP patches already in Linus' master (et al.)) comes from this line
> > from the Ubuntu commit 1a0102a0 ("UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for
> > links under VMD domain"):
> > 
> > ----
> > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> > index 00143f5fb83a..d2ff44e7fbb1 100644
> > --- a/drivers/pci/pcie/aspm.c
> > +++ b/drivers/pci/pcie/aspm.c
> > @@ -688,7 +688,8 @@ static void pcie_aspm_cap_init(struct pcie_link_state
> > *link, int blacklist)
> >         aspm_l1ss_init(link);
> > 
> >         /* Save default state */
> > -       link->aspm_default = link->aspm_enabled;
> > +       link->aspm_default = parent->dev_flags & PCI_DEV_FLAGS_ENABLE_ASPM ?
> > +                            ASPM_STATE_ALL : link->aspm_enabled;
> 
> So I thought the "pci_enable_link_state(pdev, PCIE_LINK_STATE_ALL)" in
> f492edb40b54 would effectively do the same thing:
> 
> > > > > > https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/
> > > > > > lunar/commit/?id=1a0102a08f206149d9abd56c2b28877c878b5526
> > > > > 
> > > > > This is "UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under VMD
> > > > > domain", which adds "link->aspm_default = ASPM_STATE_ALL" for device
> > > > > IDs 0x9a09 and 0xa0b0.
> > > > > 
> > > > > This looks like it should also be handled by upstream f492edb40b54
> > > > > ("PCI: vmd: Add quirk to configure PCIe ASPM and LTR") [1], which adds
> > > > > "pci_enable_link_state(pdev, PCIE_LINK_STATE_ALL)".
> 
> But I guess it doesn't actually work.  I'm hoping David or Nirmal can
> figure out why it doesn't because it seems obvious that it's the
> intent.

Is PCIe ASPM disabled? In the kernel log do you see:

    "can't override BIOS ASPM; OS doesn't have ASPM control"

David

Re: My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

[Kenneth Crudup]

I do see that:

----
[E0] 781 /usr/src/ubuntu-kernel> dmesg | fgrep -i aspm
[    0.164233] ACPI FADT declares the system doesn't support PCIe ASPM, 
so disable it
[    0.579946] acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM 
ClockPM Segments MSI EDR HPX-Type3]
[    0.587377] acpi PNP0A08:00: FADT indicates ASPM is unsupported, 
using BIOS configuration
[    1.309826] pci 10000:e0:06.0: enable ASPM for pci bridge behind vmd
[    1.622705] pci 10000:e1:00.0: can't override BIOS ASPM; OS doesn't 
have ASPM control
[110757.878494] pcieport 0000:00:07.0: ASPM: current common clock 
configuration is inconsistent, reconfiguring
[171953.284616] pcieport 0000:00:07.0: ASPM: current common clock 
configuration is inconsistent, reconfiguring
----

On 12/19/24 08:25, David E. Box wrote:
> Hi Kenneth,
> 
> On Fri, 2024-12-13 at 17:02 -0600, Bjorn Helgaas wrote:
>> [cc->to: David, Nirmal]
>>
>> On Fri, Dec 13, 2024 at 02:26:37PM -0800, Kenneth Crudup wrote:
>>> OK, it looks like the effective change (that's not already contained in the
>>> LTR SNOOP patches already in Linus' master (et al.)) comes from this line
>>> from the Ubuntu commit 1a0102a0 ("UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for
>>> links under VMD domain"):
>>>
>>> ----
>>> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
>>> index 00143f5fb83a..d2ff44e7fbb1 100644
>>> --- a/drivers/pci/pcie/aspm.c
>>> +++ b/drivers/pci/pcie/aspm.c
>>> @@ -688,7 +688,8 @@ static void pcie_aspm_cap_init(struct pcie_link_state
>>> *link, int blacklist)
>>>          aspm_l1ss_init(link);
>>>
>>>          /* Save default state */
>>> -       link->aspm_default = link->aspm_enabled;
>>> +       link->aspm_default = parent->dev_flags & PCI_DEV_FLAGS_ENABLE_ASPM ?
>>> +                            ASPM_STATE_ALL : link->aspm_enabled;
>>
>> So I thought the "pci_enable_link_state(pdev, PCIE_LINK_STATE_ALL)" in
>> f492edb40b54 would effectively do the same thing:
>>
>>>>>>> https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/
>>>>>>> lunar/commit/?id=1a0102a08f206149d9abd56c2b28877c878b5526
>>>>>>
>>>>>> This is "UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under VMD
>>>>>> domain", which adds "link->aspm_default = ASPM_STATE_ALL" for device
>>>>>> IDs 0x9a09 and 0xa0b0.
>>>>>>
>>>>>> This looks like it should also be handled by upstream f492edb40b54
>>>>>> ("PCI: vmd: Add quirk to configure PCIe ASPM and LTR") [1], which adds
>>>>>> "pci_enable_link_state(pdev, PCIE_LINK_STATE_ALL)".
>>
>> But I guess it doesn't actually work.  I'm hoping David or Nirmal can
>> figure out why it doesn't because it seems obvious that it's the
>> intent.
> 
> Is PCIe ASPM disabled? In the kernel log do you see:
> 
>      "can't override BIOS ASPM; OS doesn't have ASPM control"
> 
> David
> 

-- 
Kenneth R. Crudup / Sr. SW Engineer, Scott County Consulting, Orange 
County CA

Re: My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

[David E. Box]

+Rafael

On Thu, 2024-12-19 at 10:17 -0800, Kenneth Crudup wrote:
> I do see that:
> 
> ----
> [E0] 781 /usr/src/ubuntu-kernel> dmesg | fgrep -i aspm
> [    0.164233] ACPI FADT declares the system doesn't support PCIe ASPM, 
> so disable it

So, PCIe ASPM refers to OS control of ASPM. Disabling it means the BIOS alone
controls it, leaving the OS to stick with the defaults programmed into the
controllers by the BIOS. This might happen due to critical bugs in certain ASPM
states or simply because the OEM decided to configure it that way. We don't know
the exact reason.

The issue with VMD is that its controllers are hidden from the BIOS, so ASPM
defaults are never programmed into them. When PCIe ASPM is disabled, it's
unclear whether this should apply to controllers in VMD mode. To be cautious, we
avoid modifying ASPM settings in this scenario.

If you want to override this behavior, you can try setting pcie_aspm=force on
the kernel command line.

David

> [    0.579946] acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM 
> ClockPM Segments MSI EDR HPX-Type3]
> [    0.587377] acpi PNP0A08:00: FADT indicates ASPM is unsupported, 
> using BIOS configuration
> [    1.309826] pci 10000:e0:06.0: enable ASPM for pci bridge behind vmd
> [    1.622705] pci 10000:e1:00.0: can't override BIOS ASPM; OS doesn't 
> have ASPM control
> [110757.878494] pcieport 0000:00:07.0: ASPM: current common clock 
> configuration is inconsistent, reconfiguring
> [171953.284616] pcieport 0000:00:07.0: ASPM: current common clock 
> configuration is inconsistent, reconfiguring
> ----
> 
> On 12/19/24 08:25, David E. Box wrote:
> > Hi Kenneth,
> > 
> > On Fri, 2024-12-13 at 17:02 -0600, Bjorn Helgaas wrote:
> > > [cc->to: David, Nirmal]
> > > 
> > > On Fri, Dec 13, 2024 at 02:26:37PM -0800, Kenneth Crudup wrote:
> > > > OK, it looks like the effective change (that's not already contained in
> > > > the
> > > > LTR SNOOP patches already in Linus' master (et al.)) comes from this
> > > > line
> > > > from the Ubuntu commit 1a0102a0 ("UBUNTU: SAUCE: PCI/ASPM: Enable ASPM
> > > > for
> > > > links under VMD domain"):
> > > > 
> > > > ----
> > > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> > > > index 00143f5fb83a..d2ff44e7fbb1 100644
> > > > --- a/drivers/pci/pcie/aspm.c
> > > > +++ b/drivers/pci/pcie/aspm.c
> > > > @@ -688,7 +688,8 @@ static void pcie_aspm_cap_init(struct
> > > > pcie_link_state
> > > > *link, int blacklist)
> > > >          aspm_l1ss_init(link);
> > > > 
> > > >          /* Save default state */
> > > > -       link->aspm_default = link->aspm_enabled;
> > > > +       link->aspm_default = parent->dev_flags &
> > > > PCI_DEV_FLAGS_ENABLE_ASPM ?
> > > > +                            ASPM_STATE_ALL : link->aspm_enabled;
> > > 
> > > So I thought the "pci_enable_link_state(pdev, PCIE_LINK_STATE_ALL)" in
> > > f492edb40b54 would effectively do the same thing:
> > > 
> > > > > > > > https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/
> > > > > > > > lunar/commit/?id=1a0102a08f206149d9abd56c2b28877c878b5526
> > > > > > > 
> > > > > > > This is "UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under VMD
> > > > > > > domain", which adds "link->aspm_default = ASPM_STATE_ALL" for
> > > > > > > device
> > > > > > > IDs 0x9a09 and 0xa0b0.
> > > > > > > 
> > > > > > > This looks like it should also be handled by upstream f492edb40b54
> > > > > > > ("PCI: vmd: Add quirk to configure PCIe ASPM and LTR") [1], which
> > > > > > > adds
> > > > > > > "pci_enable_link_state(pdev, PCIE_LINK_STATE_ALL)".
> > > 
> > > But I guess it doesn't actually work.  I'm hoping David or Nirmal can
> > > figure out why it doesn't because it seems obvious that it's the
> > > intent.
> > 
> > Is PCIe ASPM disabled? In the kernel log do you see:
> > 
> >      "can't override BIOS ASPM; OS doesn't have ASPM control"
> > 
> > David
> > 
> 

Re: My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

[Kenneth Crudup]

[-- Attachment #1: Type: text/plain, Size: 525 bytes --]


Or, just use (some version of) the attached patch (against Linus' recent 
master) that enables VMD ASPM this for us "automatically" if there's a 
detected VMD ?

I'd prefer a scalpel to a bludgeon (and have been trying to get some 
version of these fixes into mainline for a while).

-Kenny

On 12/19/24 11:52, David E. Box wrote:

> If you want to override this behavior, you can try setting pcie_aspm=force on
> the kernel command line.

-- 
Kenneth R. Crudup / Sr. SW Engineer, Scott County Consulting, Orange 
County CA

[-- Attachment #2: aspm-enable-take-2 --]
[-- Type: text/plain, Size: 1557 bytes --]

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 28567d457613..a5df6230cf3c 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -768,6 +768,31 @@ static void aspm_l1ss_init(struct pcie_link_state *link)
 		aspm_calc_l12_info(link, parent_l1ss_cap, child_l1ss_cap);
 }
 
+/*
+ * BIOS may not be able to access config space of devices under VMD domain, so
+ * it relies on software to enable ASPM for links under VMD.
+ */
+static bool pci_fixup_vmd_bridge_enable_aspm(struct pci_dev *pdev)
+{
+       struct pci_bus *bus = pdev->bus;
+       struct device *dev;
+       struct pci_driver *pdrv;
+
+       if (!pci_is_root_bus(bus))
+               return false;
+
+       dev = bus->bridge->parent;
+       if (dev == NULL)
+               return false;
+
+       pdrv = pci_dev_driver(to_pci_dev(dev));
+       if (pdrv == NULL || strcmp("vmd", pdrv->name))
+               return false;
+
+       pci_info(pdev, "enable ASPM for pci bridge behind vmd");
+       return true;
+}
+
 static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 {
 	struct pci_dev *child = link->downstream, *parent = link->pdev;
@@ -846,7 +871,8 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	}
 
 	/* Save default state */
-	link->aspm_default = link->aspm_enabled;
+	link->aspm_default = pci_fixup_vmd_bridge_enable_aspm(parent) ?
+		PCIE_LINK_STATE_ASPM_ALL : link->aspm_enabled;
 
 	/* Setup initial capable state. Will be updated later */
 	link->aspm_capable = link->aspm_support;