lkml.org 
[lkml]   [2021]   [Dec]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH V4 2/2] PCI: vmd: Override ASPM on TGL/ADL VMD devices
Hello,

On Thu, Dec 16, 2021 at 1:24 PM David E. Box
<david.e.box@linux.intel.com> wrote:
>
> On Thu, 2021-12-16 at 11:26 -0600, Bjorn Helgaas wrote:
> > [+cc Rajat for LTR max latency write]

Thanks!

> >
> > On Wed, Dec 15, 2021 at 09:56:00PM -0800, David E. Box wrote:
> > > From: Michael Bottini <michael.a.bottini@linux.intel.com>
> > >
> > > On Tiger Lake and Alder Lake platforms, VMD controllers do not have ASPM
> > > enabled nor LTR values set by BIOS. This leads high power consumption on
> > > these platforms when VMD is enabled as reported in bugzilla [1]. Enable
> > > these features in the VMD driver using pcie_aspm_policy_override() to set
> > > the ASPM policy for the root ports.
> >
> > s/leads high/leads to high/
> >
> > Does this depend on "Tiger Lake and Alder Lake platforms"
> > specifically, or does it depend on a BIOS design choice, i.e., "don't
> > configure ASPM or LTR for devices below a VMD"?
>
> It was a BIOS design choice to have this done by the OS driver for both Tiger Lake and Alder Lake.
>
> >
> > The subject says "override ASPM on VMD devices," but it looks like
> > this affects the ASPM configuration of devices *below* the VMD, not of
> > the VMD itself.
>
> Yes.
>
> >
> > It looks like this only affects *NVMe* devices, since
> > vmd_enable_aspm() checks for PCI_CLASS_STORAGE_EXPRESS. Why is that?
> > Is there something special about NVMe? I'd think you would want to do
> > this for *all* devices below a VMD.
>
> We need this for all devices under the PCIe root ports below VMD. Those will only be NVMe device
> AFAICS.
>
> >
> > Since it only affects PCI_CLASS_STORAGE_EXPRESS devices, I don't think
> > it actually "sets ASPM policy for the root ports". vmd_enable_aspm()
> > calls pcie_aspm_policy_override() on endpoints. It's true that the
> > link ASPM state happens to be attached to the upstream end of the
> > link, but that's an ASPM implementation detail.
> >
> > This all needs to be clear in the subject and commit log.
>
> Ack
>
> >
> > > To do this, add an additional flag in VMD features to specify devices that
> > > must have their respective policies overridden.
> >
> > I'm not clear on why you want this to apply to only certain VMDs and
> > not others. Do some BIOSes configure ASPM for devices below some
> > VMDs?
>
> Not currently. But the plan is for future devices to move back to having BIOS do the programming.
>
> >
> > > [1] https://bugzilla.kernel.org/show_bug.cgi?id=213717
> > >
> > > Signed-off-by: Michael Bottini <michael.a.bottini@linux.intel.com>
> > > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > > Tested-by: Adhitya Mohan <me@adhityamohan.in>
> > > ---
> > > V4
> > > - Refactor vmd_enable_apsm() to exit early, making the lines shorter
> > > and more readable. Suggested by Christoph.
> > > V3
> > > - No changes
> > > V2
> > > - Use return status to print pci_info message if ASPM cannot be enabled.
> > > - Add missing static declaration, caught by lkp@intel.com
> > >
> > > drivers/pci/controller/vmd.c | 43 +++++++++++++++++++++++++++++++++---
> > > 1 file changed, 40 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> > > index a45e8e59d3d4..880afd450a14 100644
> > > --- a/drivers/pci/controller/vmd.c
> > > +++ b/drivers/pci/controller/vmd.c
> > > @@ -20,6 +20,8 @@
> > >
> > > #include <asm/irqdomain.h>
> > >
> > > +#include "../pci.h"
> > > +
> > > #define VMD_CFGBAR 0
> > > #define VMD_MEMBAR1 2
> > > #define VMD_MEMBAR2 4
> > > @@ -67,6 +69,12 @@ enum vmd_features {
> > > * interrupt handling.
> > > */
> > > VMD_FEAT_CAN_BYPASS_MSI_REMAP = (1 << 4),
> > > +
> > > + /*
> > > + * Device must have ASPM policy overridden, as its default policy is
> > > + * incorrect.
> > > + */
> > > + VMD_FEAT_QUIRK_OVERRIDE_ASPM = (1 << 5),
> >
> > I think you specifically want to *enable* some ASPM link states, not
> > just "override the default policy." "Override" tells us nothing about
> > whether you are enabling or disabling ASPM. Applies to subject line
> > as well.
>
> Ack
>
> >
> > > };
> > >
> > > static DEFINE_IDA(vmd_instance_ida);
> > > @@ -661,6 +669,30 @@ static int vmd_alloc_irqs(struct vmd_dev *vmd)
> > > return 0;
> > > }
> > >
> > > +/*
> > > + * Override the BIOS ASPM policy and set the LTR value for PCI storage
> > > + * devices on the VMD bride.
> >
> > I don't think there's any BIOS "policy" here. At this point BIOS is
> > no longer involved at all, so all that's left is whatever ASPM config
> > the BIOS did or did not do.
> >
> > Why only storage?
>
> Only storage devices will be on these root ports.
>
> >
> > s/bride/bridge/
> >
> > > + */
> > > +static int vmd_enable_aspm(struct pci_dev *pdev, void *userdata)
> > > +{
> > > + int features = *(int *)userdata, pos;
> > > +
> > > + if (!(features & VMD_FEAT_QUIRK_OVERRIDE_ASPM) ||
> > > + pdev->class != PCI_CLASS_STORAGE_EXPRESS)
> > > + return 0;
> > > +
> > > + pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_LTR);
> > > + if (!pos)
> > > + return 0;
> > > +
> > > + pci_write_config_word(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, 0x1003);
> > > + pci_write_config_word(pdev, pos + PCI_LTR_MAX_NOSNOOP_LAT, 0x1003);
> >
> > 1) Where did this magic 0x1003 value come from? Does that depend on
> > the VMD device? The endpoint? The circuit design? The path between
> > endpoint and VMD? What if there are switches in the path?
>
> The number comes from the BIOS team. They are tied to the SoC. I don't believe there can be switches
> in the path but Nirmal and Jonathan should know for sure. From what I've seen these root ports are
> wired directly to M.2 slots on boards that are intended for storage devices.
>
> >
> > 2) There exist broken devices where WORD config accesses don't work:
> > https://lore.kernel.org/all/20211208000948.487820-1-rajatja@google.com/
> >
> > We might need a way to quirk config accesses to those devices, but we
> > don't have one yet. So for now this needs to be a single DWORD write.
>
> Ack
>

Sideband: Yes, I'm trying to work out such a quirk mechanism, but for
now, yes, please do that as a single DWORD write access.

Thanks,

Rajat

> >
> > > + if (pcie_aspm_policy_override(pdev))
> > > + pci_info(pdev, "Unable of override ASPM policy\n");
> >
> > s/Unable of/Unable to/
> >
> > I think we might need a message about when we *do* override the
> > policy. A note in dmesg might be useful for debugging. I'm worried
> > about the LTR programming because I really don't understand how we
> > should be doing that.
>
> Sure.
>
> >
> > > + return 0;
> > > +}
> > > +
> > > static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
> > > {
> > > struct pci_sysdata *sd = &vmd->sysdata;
> > > @@ -807,6 +839,8 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
> > > pci_scan_child_bus(vmd->bus);
> > > pci_assign_unassigned_bus_resources(vmd->bus);
> > >
> > > + pci_walk_bus(vmd->bus, vmd_enable_aspm, &features);
> >
> > Do you support hotplug under VMD? This will not happen for hot-added
> > devices.
>
> That I don't know. Nirmal, Jonathan?
>
> David
>
> >
> > > /*
> > > * VMD root buses are virtual and don't return true on pci_is_pcie()
> > > * and will fail pcie_bus_configure_settings() early. It can instead be
> > > @@ -948,15 +982,18 @@ static const struct pci_device_id vmd_ids[] = {
> > > {PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x467f),
> > > .driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
> > > VMD_FEAT_HAS_BUS_RESTRICTIONS |
> > > - VMD_FEAT_OFFSET_FIRST_VECTOR,},
> > > + VMD_FEAT_OFFSET_FIRST_VECTOR |
> > > + VMD_FEAT_QUIRK_OVERRIDE_ASPM,},
> > > {PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x4c3d),
> > > .driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
> > > VMD_FEAT_HAS_BUS_RESTRICTIONS |
> > > - VMD_FEAT_OFFSET_FIRST_VECTOR,},
> > > + VMD_FEAT_OFFSET_FIRST_VECTOR |
> > > + VMD_FEAT_QUIRK_OVERRIDE_ASPM,},
> > > {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B),
> > > .driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
> > > VMD_FEAT_HAS_BUS_RESTRICTIONS |
> > > - VMD_FEAT_OFFSET_FIRST_VECTOR,},
> > > + VMD_FEAT_OFFSET_FIRST_VECTOR |
> > > + VMD_FEAT_QUIRK_OVERRIDE_ASPM,},
> > > {0,}
> > > };
> > > MODULE_DEVICE_TABLE(pci, vmd_ids);
> > > --
> > > 2.25.1
> > >
>
>

\
 
 \ /
  Last update: 2021-12-21 02:54    [W:0.066 / U:0.660 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site