From: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: "David E. Box" <david.e.box@linux.intel.com>,
nirmal.patel@linux.intel.com, jonathan.derrick@linux.dev,
lorenzo.pieralisi@arm.com, hch@infradead.org, kw@linux.com,
robh@kernel.org, bhelgaas@google.com,
michael.a.bottini@intel.com,
"Rafael J. Wysocki" <rafael@kernel.org>,
me@adhityamohan.in, linux-pci@vger.kernel.org,
LKML <linux-kernel@vger.kernel.org>,
intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH V2] PCI: Move VMD ASPM/LTR fix to PCI quirk
Date: Wed, 25 Oct 2023 11:38:46 +0300 (EEST) [thread overview]
Message-ID: <ab823587-1e2b-474e-f0e7-14e4782ce49f@linux.intel.com> (raw)
In-Reply-To: <ZTf6ALl3xNvhLN6M@intel.com>
[-- Attachment #1: Type: text/plain, Size: 7047 bytes --]
On Tue, 24 Oct 2023, Ville Syrjälä wrote:
> On Tue, Apr 11, 2023 at 02:33:23PM -0700, David E. Box wrote:
> > In commit f492edb40b54 ("PCI: vmd: Add quirk to configure PCIe ASPM and
> > LTR") the VMD driver calls pci_enabled_link_state as a callback from
> > pci_bus_walk. Both will acquire the pci_bus_sem lock leading to a lockdep
> > warning. Instead of doing the pci_bus_walk, move the fix to quirks.c using
> > DECLARE_PCI_FIXUP_FINAL.
>
> What happened to this patch? We're still carrying a local fix
> for this in drm-tip...
>
> >
> > Fixes: f492edb40b54 ("PCI: vmd: Add quirk to configure PCIe ASPM and LTR")
> > Suggested-by: Bjorn Helgaas <bhelgaas@google.com>
> > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > ---
> >
> > V2 - Instead of adding a lock flag argument to pci_enabled_link_state, move
> > the fix to quirks.c
> >
> > drivers/pci/controller/vmd.c | 55 +--------------------------
> > drivers/pci/quirks.c | 72 ++++++++++++++++++++++++++++++++++++
> > 2 files changed, 73 insertions(+), 54 deletions(-)
> >
> > diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> > index 990630ec57c6..47fa3e5f2dc5 100644
> > --- a/drivers/pci/controller/vmd.c
> > +++ b/drivers/pci/controller/vmd.c
> > @@ -66,22 +66,11 @@ enum vmd_features {
> > * interrupt handling.
> > */
> > VMD_FEAT_CAN_BYPASS_MSI_REMAP = (1 << 4),
> > -
> > - /*
> > - * Enable ASPM on the PCIE root ports and set the default LTR of the
> > - * storage devices on platforms where these values are not configured by
> > - * BIOS. This is needed for laptops, which require these settings for
> > - * proper power management of the SoC.
> > - */
> > - VMD_FEAT_BIOS_PM_QUIRK = (1 << 5),
> > };
> >
> > -#define VMD_BIOS_PM_QUIRK_LTR 0x1003 /* 3145728 ns */
> > -
> > #define VMD_FEATS_CLIENT (VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP | \
> > VMD_FEAT_HAS_BUS_RESTRICTIONS | \
> > - VMD_FEAT_OFFSET_FIRST_VECTOR | \
> > - VMD_FEAT_BIOS_PM_QUIRK)
> > + VMD_FEAT_OFFSET_FIRST_VECTOR)
> >
> > static DEFINE_IDA(vmd_instance_ida);
> >
> > @@ -724,46 +713,6 @@ static void vmd_copy_host_bridge_flags(struct pci_host_bridge *root_bridge,
> > vmd_bridge->native_dpc = root_bridge->native_dpc;
> > }
> >
> > -/*
> > - * Enable ASPM and LTR settings on devices that aren't configured by BIOS.
> > - */
> > -static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata)
> > -{
> > - unsigned long features = *(unsigned long *)userdata;
> > - u16 ltr = VMD_BIOS_PM_QUIRK_LTR;
> > - u32 ltr_reg;
> > - int pos;
> > -
> > - if (!(features & VMD_FEAT_BIOS_PM_QUIRK))
> > - return 0;
> > -
> > - pci_enable_link_state(pdev, PCIE_LINK_STATE_ALL);
> > -
> > - pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_LTR);
> > - if (!pos)
> > - return 0;
> > -
> > - /*
> > - * Skip if the max snoop LTR is non-zero, indicating BIOS has set it
> > - * so the LTR quirk is not needed.
> > - */
> > - pci_read_config_dword(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, <r_reg);
> > - if (!!(ltr_reg & (PCI_LTR_VALUE_MASK | PCI_LTR_SCALE_MASK)))
> > - return 0;
> > -
> > - /*
> > - * Set the default values to the maximum required by the platform to
> > - * allow the deepest power management savings. Write as a DWORD where
> > - * the lower word is the max snoop latency and the upper word is the
> > - * max non-snoop latency.
> > - */
> > - ltr_reg = (ltr << 16) | ltr;
> > - pci_write_config_dword(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, ltr_reg);
> > - pci_info(pdev, "VMD: Default LTR value set by driver\n");
> > -
> > - return 0;
> > -}
> > -
> > static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
> > {
> > struct pci_sysdata *sd = &vmd->sysdata;
> > @@ -936,8 +885,6 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
> >
> > pci_assign_unassigned_bus_resources(vmd->bus);
> >
> > - pci_walk_bus(vmd->bus, vmd_pm_enable_quirk, &features);
> > -
> > /*
> > * 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
> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > index 44cab813bf95..2d86623f96e3 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -6023,3 +6023,75 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x9a2d, dpc_log_size);
> > DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x9a2f, dpc_log_size);
> > DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x9a31, dpc_log_size);
> > #endif
> > +
> > +#ifdef CONFIG_VMD
> > +/*
> > + * Enable ASPM on the PCIE root ports under VMD and set the default LTR of the
> > + * storage devices on platforms where these values are not configured by BIOS.
> > + * This is needed for laptops, which require these settings for proper power
> > + * management of the SoC.
> > + */
> > +#define VMD_DEVICE_LTR 0x1003 /* 3145728 ns */
This should be defined using FIELD_PREP()s. It is better to construct both
snoop and nosnoop registers here and not do the shift below at all.
There are new defines for the nosnoop reg in pci/field-get branch for the
nosnoop reg fields.
--
i.
> > +static void quirk_intel_vmd(struct pci_dev *pdev)
> > +{
> > + struct pci_dev *parent;
> > + u16 ltr = VMD_DEVICE_LTR;
> > + u32 ltr_reg;
> > + int pos;
> > +
> > + /* Check in VMD domain */
> > + if (pci_domain_nr(pdev->bus) < 0x10000)
> > + return;
> > +
> > + /* Get Root Port */
> > + parent = pci_upstream_bridge(pdev);
> > + if (!parent || parent->vendor != PCI_VENDOR_ID_INTEL)
> > + return;
> > +
> > + /* Get VMD Host Bridge */
> > + parent = to_pci_dev(parent->dev.parent);
> > + if (!parent)
> > + return;
> > +
> > + /* Get RAID controller */
> > + parent = to_pci_dev(parent->dev.parent);
> > + if (!parent)
> > + return;
> > +
> > + switch (parent->device) {
> > + case 0x467f:
> > + case 0x4c3d:
> > + case 0xa77f:
> > + case 0x7d0b:
> > + case 0xad0b:
> > + case 0x9a0b:
> > + break;
> > + default:
> > + return;
> > + }
> > +
> > + pci_enable_link_state(pdev, PCIE_LINK_STATE_ALL);
> > +
> > + pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_LTR);
> > + if (!pos)
> > + return;
> > +
> > + /* Skip if the max snoop LTR is non-zero, indicating BIOS has set it */
> > + pci_read_config_dword(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, <r_reg);
> > + if (!!(ltr_reg & (PCI_LTR_VALUE_MASK | PCI_LTR_SCALE_MASK)))
> > + return;
> > +
> > + /*
> > + * Set the LTR values to the maximum required by the platform to
> > + * allow the deepest power management savings. Write as a DWORD where
> > + * the lower word is the max snoop latency and the upper word is the
> > + * max non-snoop latency.
> > + */
> > + ltr_reg = (ltr << 16) | ltr;
> > + pci_write_config_dword(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, ltr_reg);
> > + pci_info(pdev, "LTR set by VMD PCI quick\n");
> > +
> > +}
> > +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_ANY_ID, PCI_ANY_ID,
> > + PCI_CLASS_STORAGE_EXPRESS, 0, quirk_intel_vmd);
> > +#endif
> > --
> > 2.34.1
>
>
prev parent reply other threads:[~2023-10-25 8:49 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-04-11 21:33 [PATCH V2] PCI: Move VMD ASPM/LTR fix to PCI quirk David E. Box
2023-06-08 20:52 ` [Intel-gfx] " Bjorn Helgaas
2023-06-09 22:09 ` David E. Box
2023-06-09 22:46 ` Bjorn Helgaas
2023-06-10 0:09 ` David E. Box
2023-10-24 17:08 ` Ville Syrjälä
2023-10-25 8:38 ` Ilpo Järvinen [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ab823587-1e2b-474e-f0e7-14e4782ce49f@linux.intel.com \
--to=ilpo.jarvinen@linux.intel.com \
--cc=bhelgaas@google.com \
--cc=david.e.box@linux.intel.com \
--cc=hch@infradead.org \
--cc=intel-gfx@lists.freedesktop.org \
--cc=jonathan.derrick@linux.dev \
--cc=kw@linux.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=lorenzo.pieralisi@arm.com \
--cc=me@adhityamohan.in \
--cc=michael.a.bottini@intel.com \
--cc=nirmal.patel@linux.intel.com \
--cc=rafael@kernel.org \
--cc=robh@kernel.org \
--cc=ville.syrjala@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).