* [PATCH 1/2] PCI/ASPM: Add ASPM BIOS override function @ 2021-11-20 1:57 David E. Box 2021-11-20 1:57 ` [PATCH 2/2] PCI: vmd: Override ASPM on TGL/ADL VMD devices David E. Box 2021-11-30 8:03 ` [PATCH 1/2] PCI/ASPM: Add ASPM BIOS override function Krzysztof Wilczyński 0 siblings, 2 replies; 5+ messages in thread From: David E. Box @ 2021-11-20 1:57 UTC (permalink / raw) To: nirmal.patel, jonathan.derrick, lorenzo.pieralisi, robh, kw, bhelgaas, david.e.box, michael.a.bottini, rafael Cc: linux-pci, linux-kernel From: Michael Bottini <michael.a.bottini@linux.intel.com> Devices that appear under the Intel VMD host bridge are not visible to BIOS and therefore not programmed by BIOS with ASPM settings. For these devices, it is necessary for the driver to configure ASPM. Since ASPM settings are adjustable at runtime by module parameter, use the same mechanism to allow drivers to override the default (in this case never configured) BIOS policy to ASPM_STATE_ALL. Then, reconfigure ASPM on the link. Signed-off-by: Michael Bottini <michael.a.bottini@linux.intel.com> Signed-off-by: David E. Box <david.e.box@linux.intel.com> --- drivers/pci/pci.h | 2 ++ drivers/pci/pcie/aspm.c | 18 ++++++++++++++++++ 2 files changed, 20 insertions(+) diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index 3d60cabde1a1..172ec914e988 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h @@ -562,11 +562,13 @@ 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); void pcie_aspm_powersave_config_link(struct pci_dev *pdev); +void pcie_aspm_policy_override(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) { } static inline void pcie_aspm_powersave_config_link(struct pci_dev *pdev) { } +static inline void pcie_aspm_policy_override(struct pci_dev *dev) {} #endif #ifdef CONFIG_PCIE_ECRC diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c index 52c74682601a..ccb98586bf0d 100644 --- a/drivers/pci/pcie/aspm.c +++ b/drivers/pci/pcie/aspm.c @@ -1140,6 +1140,24 @@ int pci_disable_link_state(struct pci_dev *pdev, int state) } EXPORT_SYMBOL(pci_disable_link_state); +void pcie_aspm_policy_override(struct pci_dev *pdev) +{ + struct pcie_link_state *link = pcie_aspm_get_link(pdev); + + down_read(&pci_bus_sem); + mutex_lock(&aspm_lock); + + if (link) { + link->aspm_default = ASPM_STATE_ALL; + pcie_config_aspm_link(link, policy_to_aspm_state(link)); + pcie_set_clkpm(link, policy_to_clkpm_state(link)); + } + + mutex_unlock(&aspm_lock); + up_read(&pci_bus_sem); +} +EXPORT_SYMBOL(pcie_aspm_policy_override); + static int pcie_aspm_set_policy(const char *val, const struct kernel_param *kp) { -- 2.25.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/2] PCI: vmd: Override ASPM on TGL/ADL VMD devices 2021-11-20 1:57 [PATCH 1/2] PCI/ASPM: Add ASPM BIOS override function David E. Box @ 2021-11-20 1:57 ` David E. Box 2021-11-20 5:13 ` kernel test robot 2021-11-20 16:35 ` kernel test robot 2021-11-30 8:03 ` [PATCH 1/2] PCI/ASPM: Add ASPM BIOS override function Krzysztof Wilczyński 1 sibling, 2 replies; 5+ messages in thread From: David E. Box @ 2021-11-20 1:57 UTC (permalink / raw) To: nirmal.patel, jonathan.derrick, lorenzo.pieralisi, robh, kw, bhelgaas, david.e.box, michael.a.bottini, rafael Cc: linux-pci, linux-kernel 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. To do this, add an additional flag in VMD features to specify devices that must have their respective policies overridden. [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> --- drivers/pci/controller/vmd.c | 40 +++++++++++++++++++++++++++++++++--- 1 file changed, 37 insertions(+), 3 deletions(-) diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c index a45e8e59d3d4..0d0b8888cd90 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), }; static DEFINE_IDA(vmd_instance_ida); @@ -661,6 +669,27 @@ 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. + */ +int vmd_enable_aspm(struct pci_dev *pdev, void *userdata) +{ + int features = *(int *)userdata; + + if (features & VMD_FEAT_QUIRK_OVERRIDE_ASPM && + pdev->class == PCI_CLASS_STORAGE_EXPRESS) { + int pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_LTR); + + if (pos) { + pci_write_config_word(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, 0x1003); + pci_write_config_word(pdev, pos + PCI_LTR_MAX_NOSNOOP_LAT, 0x1003); + pcie_aspm_policy_override(pdev); + } + } + return 0; +} + static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features) { struct pci_sysdata *sd = &vmd->sysdata; @@ -807,6 +836,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); + /* * 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 +979,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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] PCI: vmd: Override ASPM on TGL/ADL VMD devices 2021-11-20 1:57 ` [PATCH 2/2] PCI: vmd: Override ASPM on TGL/ADL VMD devices David E. Box @ 2021-11-20 5:13 ` kernel test robot 2021-11-20 16:35 ` kernel test robot 1 sibling, 0 replies; 5+ messages in thread From: kernel test robot @ 2021-11-20 5:13 UTC (permalink / raw) To: David E. Box, nirmal.patel, jonathan.derrick, lorenzo.pieralisi, robh, kw, bhelgaas, michael.a.bottini, rafael Cc: kbuild-all, linux-pci [-- Attachment #1: Type: text/plain, Size: 2370 bytes --] Hi "David, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on helgaas-pci/next] [also build test WARNING on v5.16-rc1 next-20211118] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/David-E-Box/PCI-ASPM-Add-ASPM-BIOS-override-function/20211120-095959 base: https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next config: x86_64-randconfig-a012-20211118 (attached as .config) compiler: gcc-9 (Debian 9.3.0-22) 9.3.0 reproduce (this is a W=1 build): # https://github.com/0day-ci/linux/commit/d0452407e2d5bf22bd1094654d7e868311b7c94e git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review David-E-Box/PCI-ASPM-Add-ASPM-BIOS-override-function/20211120-095959 git checkout d0452407e2d5bf22bd1094654d7e868311b7c94e # save the attached .config to linux build tree make W=1 ARCH=x86_64 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All warnings (new ones prefixed by >>): >> drivers/pci/controller/vmd.c:676:5: warning: no previous prototype for 'vmd_enable_aspm' [-Wmissing-prototypes] 676 | int vmd_enable_aspm(struct pci_dev *pdev, void *userdata) | ^~~~~~~~~~~~~~~ vim +/vmd_enable_aspm +676 drivers/pci/controller/vmd.c 671 672 /* 673 * Override the BIOS ASPM policy and set the LTR value for PCI storage 674 * devices on the VMD bride. 675 */ > 676 int vmd_enable_aspm(struct pci_dev *pdev, void *userdata) 677 { 678 int features = *(int *)userdata; 679 680 if (features & VMD_FEAT_QUIRK_OVERRIDE_ASPM && 681 pdev->class == PCI_CLASS_STORAGE_EXPRESS) { 682 int pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_LTR); 683 684 if (pos) { 685 pci_write_config_word(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, 0x1003); 686 pci_write_config_word(pdev, pos + PCI_LTR_MAX_NOSNOOP_LAT, 0x1003); 687 pcie_aspm_policy_override(pdev); 688 } 689 } 690 return 0; 691 } 692 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 41220 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] PCI: vmd: Override ASPM on TGL/ADL VMD devices 2021-11-20 1:57 ` [PATCH 2/2] PCI: vmd: Override ASPM on TGL/ADL VMD devices David E. Box 2021-11-20 5:13 ` kernel test robot @ 2021-11-20 16:35 ` kernel test robot 1 sibling, 0 replies; 5+ messages in thread From: kernel test robot @ 2021-11-20 16:35 UTC (permalink / raw) To: David E. Box, nirmal.patel, jonathan.derrick, lorenzo.pieralisi, robh, kw, bhelgaas, michael.a.bottini, rafael Cc: llvm, kbuild-all, linux-pci [-- Attachment #1: Type: text/plain, Size: 2752 bytes --] Hi "David, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on helgaas-pci/next] [also build test WARNING on v5.16-rc1 next-20211118] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/David-E-Box/PCI-ASPM-Add-ASPM-BIOS-override-function/20211120-095959 base: https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next config: x86_64-buildonly-randconfig-r001-20211118 (attached as .config) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/d0452407e2d5bf22bd1094654d7e868311b7c94e git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review David-E-Box/PCI-ASPM-Add-ASPM-BIOS-override-function/20211120-095959 git checkout d0452407e2d5bf22bd1094654d7e868311b7c94e # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=x86_64 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All warnings (new ones prefixed by >>): >> drivers/pci/controller/vmd.c:676:5: warning: no previous prototype for function 'vmd_enable_aspm' [-Wmissing-prototypes] int vmd_enable_aspm(struct pci_dev *pdev, void *userdata) ^ drivers/pci/controller/vmd.c:676:1: note: declare 'static' if the function is not intended to be used outside of this translation unit int vmd_enable_aspm(struct pci_dev *pdev, void *userdata) ^ static 1 warning generated. vim +/vmd_enable_aspm +676 drivers/pci/controller/vmd.c 671 672 /* 673 * Override the BIOS ASPM policy and set the LTR value for PCI storage 674 * devices on the VMD bride. 675 */ > 676 int vmd_enable_aspm(struct pci_dev *pdev, void *userdata) 677 { 678 int features = *(int *)userdata; 679 680 if (features & VMD_FEAT_QUIRK_OVERRIDE_ASPM && 681 pdev->class == PCI_CLASS_STORAGE_EXPRESS) { 682 int pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_LTR); 683 684 if (pos) { 685 pci_write_config_word(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, 0x1003); 686 pci_write_config_word(pdev, pos + PCI_LTR_MAX_NOSNOOP_LAT, 0x1003); 687 pcie_aspm_policy_override(pdev); 688 } 689 } 690 return 0; 691 } 692 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 41769 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] PCI/ASPM: Add ASPM BIOS override function 2021-11-20 1:57 [PATCH 1/2] PCI/ASPM: Add ASPM BIOS override function David E. Box 2021-11-20 1:57 ` [PATCH 2/2] PCI: vmd: Override ASPM on TGL/ADL VMD devices David E. Box @ 2021-11-30 8:03 ` Krzysztof Wilczyński 1 sibling, 0 replies; 5+ messages in thread From: Krzysztof Wilczyński @ 2021-11-30 8:03 UTC (permalink / raw) To: David E. Box Cc: nirmal.patel, jonathan.derrick, lorenzo.pieralisi, robh, bhelgaas, michael.a.bottini, rafael, linux-pci, linux-kernel Hi David, [...] > @@ -562,11 +562,13 @@ 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); > void pcie_aspm_powersave_config_link(struct pci_dev *pdev); > +void pcie_aspm_policy_override(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) { } > static inline void pcie_aspm_powersave_config_link(struct pci_dev *pdev) { } > +static inline void pcie_aspm_policy_override(struct pci_dev *dev) {} > #endif A small nitpick, and a slight OCD on my part, so feel free to ignore this, of course: a missing space between curly brackets, to keep things aligned with previous definitions. > @@ -1140,6 +1140,24 @@ int pci_disable_link_state(struct pci_dev *pdev, int state) > } > EXPORT_SYMBOL(pci_disable_link_state); > > +void pcie_aspm_policy_override(struct pci_dev *pdev) > +{ > + struct pcie_link_state *link = pcie_aspm_get_link(pdev); > + > + down_read(&pci_bus_sem); > + mutex_lock(&aspm_lock); > + > + if (link) { > + link->aspm_default = ASPM_STATE_ALL; > + pcie_config_aspm_link(link, policy_to_aspm_state(link)); > + pcie_set_clkpm(link, policy_to_clkpm_state(link)); > + } > + > + mutex_unlock(&aspm_lock); > + up_read(&pci_bus_sem); > +} > +EXPORT_SYMBOL(pcie_aspm_policy_override); What about the following version where if we have no link (albeit, I am not sure how often this is going to be the case?) then we don't even attempt to get a hold on the lock and such: void pcie_aspm_policy_override(struct pci_dev *pdev) { struct pcie_link_state *link = pcie_aspm_get_link(pdev); if (!link) return; down_read(&pci_bus_sem); mutex_lock(&aspm_lock); link->aspm_default = ASPM_STATE_ALL; pcie_config_aspm_link(link, policy_to_aspm_state(link)); pcie_set_clkpm(link, policy_to_clkpm_state(link)); mutex_unlock(&aspm_lock); up_read(&pci_bus_sem); } What do you think? Would this make sense? Krzysztof ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-11-30 8:03 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-11-20 1:57 [PATCH 1/2] PCI/ASPM: Add ASPM BIOS override function David E. Box 2021-11-20 1:57 ` [PATCH 2/2] PCI: vmd: Override ASPM on TGL/ADL VMD devices David E. Box 2021-11-20 5:13 ` kernel test robot 2021-11-20 16:35 ` kernel test robot 2021-11-30 8:03 ` [PATCH 1/2] PCI/ASPM: Add ASPM BIOS override function Krzysztof Wilczyński
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox