From: Bjorn Helgaas <helgaas@kernel.org>
To: "David E. Box" <david.e.box@linux.intel.com>
Cc: ville.syrjala@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@kernel.org, me@adhityamohan.in, linux-pci@vger.kernel.org,
intel-gfx@lists.freedesktop.org, linux-kernel@vger.kernel.org
Subject: Re: [Intel-gfx] [PATCH V2] PCI: Move VMD ASPM/LTR fix to PCI quirk
Date: Thu, 8 Jun 2023 15:52:08 -0500 [thread overview]
Message-ID: <20230608205208.GA1214785@bhelgaas> (raw)
In-Reply-To: <20230411213323.1362300-1-david.e.box@linux.intel.com>
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.
s/pci_enabled_link_state/pci_enable_link_state/
Add "()" after pci_enable_link_state() and pci_bus_walk() to make it
obvious they're functions.
> ...
> +++ 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.
s/PCIE/PCIe/ to match spec usage.
> + */
> +#define VMD_DEVICE_LTR 0x1003 /* 3145728 ns */
It would be nice to know how this value was derived. But I know we
had this hard-coded value before, so it's not new with this patch.
> +static void quirk_intel_vmd(struct pci_dev *pdev)
I think this quirk could possibly stay in
drivers/pci/controller/vmd.c, couldn't it? It has a lot of
VMD-specific knowledge that it would nice to contain in vmd.c.
> +{
> + struct pci_dev *parent;
> + u16 ltr = VMD_DEVICE_LTR;
I don't think "ltr" is an improvement over using "VMD_DEVICE_LTR"
below.
> + u32 ltr_reg;
> + int pos;
> +
> + /* Check in VMD domain */
> + if (pci_domain_nr(pdev->bus) < 0x10000)
> + return;
If in vmd.c, maybe could identify devices under a VMD by checking
pdev->bus->ops as vmd_acpi_find_companion() does?
> + /* 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);
Seems like you would want to set LTR *before* enabling the link
states?
> + 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.
This comment suggests that the LTR value is platform-dependent, which
is what I would expect, but the code and the hard-coded VMD_DEVICE_LTR
value don't have any platform dependencies. Again, nothing new in
*this* patch; that's true in the current tree, too.
> + 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");
s/quick/quirk/
> +
> +}
> +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_ANY_ID, PCI_ANY_ID,
> + PCI_CLASS_STORAGE_EXPRESS, 0, quirk_intel_vmd);
> +#endif
> --
> 2.34.1
>
next prev parent reply other threads:[~2023-06-08 20:52 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 ` Bjorn Helgaas [this message]
2023-06-09 22:09 ` [Intel-gfx] " 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
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=20230608205208.GA1214785@bhelgaas \
--to=helgaas@kernel.org \
--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).