On Thu, Jul 24, 2025 at 12:08:03PM GMT, David Box wrote: > On Thu, Jul 24, 2025 at 10:18:47PM +0530, Manivannan Sadhasivam wrote: > > On Thu, Jul 24, 2025 at 11:58:40AM GMT, Rafael J. Wysocki wrote: > > > On Wed, Jul 23, 2025 at 11:27 PM David Box <david.e.box@xxxxxxxxxxxxxxx> wrote: > > > > > > > > On Wed, Jul 23, 2025 at 01:54:41PM +0200, Rafael J. Wysocki wrote: > > > > > On Mon, Jul 21, 2025 at 10:24 AM Manivannan Sadhasivam <mani@xxxxxxxxxx> wrote: > > > > > > > > > > > > On Sun, Jul 20, 2025 at 12:01:37PM GMT, David E. Box wrote: > > > > > > > Synthetic PCIe hierarchies, such as those created by Intel VMD, are not > > > > > > > visible to firmware and do not receive BIOS-provided default ASPM and CLKPM > > > > > > > configuration. As a result, devices behind such domains operate without > > > > > > > proper power management, regardless of platform intent. > > > > > > > > > > > > > > To address this, allow controller drivers to supply an override for the > > > > > > > default link state by setting aspm_dflt_link_state for their associated > > > > > > > pci_host_bridge. During link initialization, if this field is non-zero, > > > > > > > ASPM and CLKPM defaults are derived from its value instead of being taken > > > > > > > from BIOS. > > > > > > > > > > > > > > This mechanism enables drivers like VMD to achieve platform-aligned power > > > > > > > savings by statically defining the expected link configuration at > > > > > > > enumeration time, without relying on runtime calls such as > > > > > > > pci_enable_link_state(), which are ineffective when ASPM is disabled > > > > > > > globally. > > > > > > > > > > > > > > This approach avoids per-controller hacks in ASPM core logic and provides a > > > > > > > general mechanism for domains that require explicit control over link power > > > > > > > state defaults. > > > > > > > > > > > > > > Link: https://lore.kernel.org/linux-pm/0b166ece-eeec-ba5d-2212-50d995611cef@xxxxxxxxx > > > > > > > Signed-off-by: David E. Box <david.e.box@xxxxxxxxxxxxxxx> > > > > > > > --- > > > > > > > > > > > > > > Changes from RFC: > > > > > > > > > > > > > > -- Rename field to aspm_dflt_link_state since it stores > > > > > > > PCIE_LINK_STATE_XXX flags, not a policy enum. > > > > > > > -- Move the field to struct pci_host_bridge since it's being applied to > > > > > > > the entire host bridge per Mani's suggestion. > > > > > > > -- During testing noticed that clkpm remained disabled and this was > > > > > > > also handled by the formerly used pci_enable_link_state(). Add a > > > > > > > check in pcie_clkpm_cap_init() as well to enable clkpm during init. > > > > > > > > > > > > > > drivers/pci/controller/vmd.c | 12 +++++++++--- > > > > > > > drivers/pci/pcie/aspm.c | 13 +++++++++++-- > > > > > > > include/linux/pci.h | 4 ++++ > > > > > > > 3 files changed, 24 insertions(+), 5 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c > > > > > > > index 8df064b62a2f..6f0de95c87fd 100644 > > > > > > > --- a/drivers/pci/controller/vmd.c > > > > > > > +++ b/drivers/pci/controller/vmd.c > > > > > > > @@ -730,7 +730,7 @@ static void vmd_copy_host_bridge_flags(struct pci_host_bridge *root_bridge, > > > > > > > } > > > > > > > > > > > > > > /* > > > > > > > - * Enable ASPM and LTR settings on devices that aren't configured by BIOS. > > > > > > > + * Enable LTR settings on devices that aren't configured by BIOS. > > > > > > > */ > > > > > > > static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata) > > > > > > > { > > > > > > > @@ -770,7 +770,6 @@ static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata) > > > > > > > * PCIe r6.0, sec 5.5.4. > > > > > > > */ > > > > > > > pci_set_power_state_locked(pdev, PCI_D0); > > > > > > > > > > > > This call becomes useless now. > > > > > > > > Missed this. I'll remove it. > > > > > > > > > > > > > > > > > - pci_enable_link_state_locked(pdev, PCIE_LINK_STATE_ALL); > > > > > > > return 0; > > > > > > > } > > > > > > > > > > > > > > @@ -785,6 +784,7 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features) > > > > > > > resource_size_t membar2_offset = 0x2000; > > > > > > > struct pci_bus *child; > > > > > > > struct pci_dev *dev; > > > > > > > + struct pci_host_bridge *vmd_host_bridge; > > > > > > > int ret; > > > > > > > > > > > > > > /* > > > > > > > @@ -911,8 +911,14 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features) > > > > > > > return -ENODEV; > > > > > > > } > > > > > > > > > > > > > > + vmd_host_bridge = to_pci_host_bridge(vmd->bus->bridge); > > > > > > > + > > > > > > > +#ifdef CONFIG_PCIEASPM > > > > > > > + vmd_host_bridge->aspm_dflt_link_state = PCIE_LINK_STATE_ALL; > > > > > > > +#endif > > > > > > > > > > > > I think it is better to provide an API that accepts the link state. We can > > > > > > provide a stub if CONFIG_PCIEASPM is not selected. This will avoid the ifdef > > > > > > clutter in the callers. Like: > > > > > > > > > > > > void pci_set_default_link_state(struct pci_host_bridge *host_bridge, > > > > > > unsigned int state) > > > > > > { > > > > > > #ifdef CONFIG_PCIEASPM > > > > > > host_bridge->aspm_default_link_state = state; > > > > > > #endif > > > > > > } > > > > > > > > > > > > Or you can stub the entire function to align with other ASPM APIs. > > > > > > > > > > > > One more thought: Since this API is only going to be called by the host bridge > > > > > > drivers, we can place it in drivers/pci/controller/pci-host-common.c and name it > > > > > > as pci_host_common_set_default_link_state(). > > > > > > > > This would require VMD to select PCI_HOST_COMMON just to set one field in a > > > > common struct. Seems heavy-handed. Thoughts? Also, with this and dropping the D0 > > > > call, I'll split the VMD cleanup into a separate patch again. > > > > > > So maybe define a __weak pci_host_set_default_pcie_link_state() doing > > > nothing in the ASPM core and let VMD override it with its own > > > implementation? > > > > > > > No. There are other controller drivers (like pcie-qcom) going to use this API. > > So please move it to the pci-host-common library as it should be. > > I was going to suggest that it could simply stay in aspm.c. > pci_enable_link_state_() is there and currently only used by controllers as > well. > OK! - Mani -- மணிவண்ணன் சதாசிவம்
