| Message ID | 20200728161321.38229-1-jonathan.derrick@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [RFC] PCI: vmd: Enable ASPM if BIOS requests it | expand |
On Tue, Jul 28, 2020 at 12:13:21PM -0400, Jon Derrick wrote: > VMD domains are not ACPI-managed devices and do not have the necessary > ACPI hooks to enable ASPM. However if the BIOS has requested ASPM > enablement, we should try to honor that request regardless. This patch > adds the ASPM support to VMD child devices if requested by the FADT > table. ASPM is enabled by software but the actual link state transitions between L0, L0s, L1, etc are done autonomously by hardware. There should be no functional difference between ASPM being disabled vs being enabled. The only difference should be power consumption and some latency. There are no ACPI hooks required to enable ASPM. The ACPI_FADT_NO_ASPM bit tells the OS that "it must not enable OSPM ASPM control" (ACPI v6.3, sec 5.2.9.3). AFAIK there is nothing in ACPI to request that the OS *should* enable ASPM. > Signed-off-by: Jon Derrick <jonathan.derrick@intel.com> > Signed-off-by: You-Sheng Yang <vicamo.yang@canonical.com> > --- > > Hi, > > My knowledge on these kinds of power modes is limited, and we are having > trouble bringing the Root Port child device out of L1 with this patch. Presumably this patch helps something. Do you mean you're having trouble *without* this patch? > Can you help me understand the correct flow for bringing the Root Port > device out of L1 with kernel flow, and what I might be missing here? I don't understand the issue yet. L1 is a state of the link, not of an individual device. The devices on both ends of the link, e.g., a Root Port and an Endpoint or Switch Upstream Port below it, negotiate to determine the link state. This all happens in hardware without software involvement. The only software influence is to enable hardware to select certain link states. > drivers/pci/controller/vmd.c | 9 ++++++++- > drivers/pci/pcie/aspm.c | 19 ++----------------- > include/linux/pci.h | 17 +++++++++++++++++ > 3 files changed, 27 insertions(+), 18 deletions(-) > > diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c > index 76d8acbee7d5..f1b058efb642 100644 > --- a/drivers/pci/controller/vmd.c > +++ b/drivers/pci/controller/vmd.c > @@ -14,6 +14,7 @@ > #include <linux/srcu.h> > #include <linux/rculist.h> > #include <linux/rcupdate.h> > +#include <linux/acpi.h> > > #include <asm/irqdomain.h> > #include <asm/device.h> > @@ -601,8 +602,14 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features) > * and will fail pcie_bus_configure_settings() early. It can instead be > * run on each of the real root ports. > */ > - list_for_each_entry(child, &vmd->bus->children, node) > + list_for_each_entry(child, &vmd->bus->children, node) { > +#if IS_ENABLED(CONFIG_PCIEASPM) > + if (!(acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_ASPM)) > + pcie_config_aspm_link(child->self->link_state, > + ASPM_STATE_ALL); > +#endif pcie_aspm_init_link_state() is called from pci_scan_slot() and should be doing ASPM configuration without help from VMD. If that's not happening, I would instrument pcie_aspm_init_link_state() to make sure it's being called and to figure out if there's something there that prevents ASPM config for children of the VMD device. > pcie_bus_configure_settings(child); > + } > > pci_bus_add_devices(vmd->bus); > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c > index 253c30cc1967..04cdb0b5a672 100644 > --- a/drivers/pci/pcie/aspm.c > +++ b/drivers/pci/pcie/aspm.c > @@ -25,22 +25,6 @@ > #endif > #define MODULE_PARAM_PREFIX "pcie_aspm." > > -/* Note: those are not register definitions */ > -#define ASPM_STATE_L0S_UP (1) /* Upstream direction L0s state */ > -#define ASPM_STATE_L0S_DW (2) /* Downstream direction L0s state */ > -#define ASPM_STATE_L1 (4) /* L1 state */ > -#define ASPM_STATE_L1_1 (8) /* ASPM L1.1 state */ > -#define ASPM_STATE_L1_2 (0x10) /* ASPM L1.2 state */ > -#define ASPM_STATE_L1_1_PCIPM (0x20) /* PCI PM L1.1 state */ > -#define ASPM_STATE_L1_2_PCIPM (0x40) /* PCI PM L1.2 state */ > -#define ASPM_STATE_L1_SS_PCIPM (ASPM_STATE_L1_1_PCIPM | ASPM_STATE_L1_2_PCIPM) > -#define ASPM_STATE_L1_2_MASK (ASPM_STATE_L1_2 | ASPM_STATE_L1_2_PCIPM) > -#define ASPM_STATE_L1SS (ASPM_STATE_L1_1 | ASPM_STATE_L1_1_PCIPM |\ > - ASPM_STATE_L1_2_MASK) > -#define ASPM_STATE_L0S (ASPM_STATE_L0S_UP | ASPM_STATE_L0S_DW) > -#define ASPM_STATE_ALL (ASPM_STATE_L0S | ASPM_STATE_L1 | \ > - ASPM_STATE_L1SS) > - > struct aspm_latency { > u32 l0s; /* L0s latency (nsec) */ > u32 l1; /* L1 latency (nsec) */ > @@ -748,7 +732,7 @@ static void pcie_config_aspm_dev(struct pci_dev *pdev, u32 val) > PCI_EXP_LNKCTL_ASPMC, val); > } > > -static void pcie_config_aspm_link(struct pcie_link_state *link, u32 state) > +void pcie_config_aspm_link(struct pcie_link_state *link, u32 state) > { > u32 upstream = 0, dwstream = 0; > struct pci_dev *child = link->downstream, *parent = link->pdev; > @@ -798,6 +782,7 @@ static void pcie_config_aspm_link(struct pcie_link_state *link, u32 state) > > link->aspm_enabled = state; > } > +EXPORT_SYMBOL_GPL(pcie_config_aspm_link); > > static void pcie_config_aspm_path(struct pcie_link_state *link) > { > diff --git a/include/linux/pci.h b/include/linux/pci.h > index 7a40cd5caed0..1c41781b160a 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -377,6 +377,22 @@ struct pci_dev { > unsigned int d3cold_delay; /* D3cold->D0 transition time in ms */ > > #ifdef CONFIG_PCIEASPM > +/* Note: those are not register definitions */ > +#define ASPM_STATE_L0S_UP (1) /* Upstream direction L0s state */ > +#define ASPM_STATE_L0S_DW (2) /* Downstream direction L0s state */ > +#define ASPM_STATE_L1 (4) /* L1 state */ > +#define ASPM_STATE_L1_1 (8) /* ASPM L1.1 state */ > +#define ASPM_STATE_L1_2 (0x10) /* ASPM L1.2 state */ > +#define ASPM_STATE_L1_1_PCIPM (0x20) /* PCI PM L1.1 state */ > +#define ASPM_STATE_L1_2_PCIPM (0x40) /* PCI PM L1.2 state */ > +#define ASPM_STATE_L1_SS_PCIPM (ASPM_STATE_L1_1_PCIPM | ASPM_STATE_L1_2_PCIPM) > +#define ASPM_STATE_L1_2_MASK (ASPM_STATE_L1_2 | ASPM_STATE_L1_2_PCIPM) > +#define ASPM_STATE_L1SS (ASPM_STATE_L1_1 | ASPM_STATE_L1_1_PCIPM |\ > + ASPM_STATE_L1_2_MASK) > +#define ASPM_STATE_L0S (ASPM_STATE_L0S_UP | ASPM_STATE_L0S_DW) > +#define ASPM_STATE_ALL (ASPM_STATE_L0S | ASPM_STATE_L1 | \ > + ASPM_STATE_L1SS) > + > struct pcie_link_state *link_state; /* ASPM link state */ > unsigned int ltr_path:1; /* Latency Tolerance Reporting > supported from root to here */ > @@ -1577,6 +1593,7 @@ extern bool pcie_ports_native; > #define PCIE_LINK_STATE_L1_2_PCIPM BIT(6) > > #ifdef CONFIG_PCIEASPM > +void pcie_config_aspm_link(struct pcie_link_state *link, u32 state); > int pci_disable_link_state(struct pci_dev *pdev, int state); > int pci_disable_link_state_locked(struct pci_dev *pdev, int state); > void pcie_no_aspm(void); > -- > 2.18.1 >
Sorry for the delay. New patch incoming On Tue, 2020-07-28 at 12:04 -0500, Bjorn Helgaas wrote: > On Tue, Jul 28, 2020 at 12:13:21PM -0400, Jon Derrick wrote: > > VMD domains are not ACPI-managed devices and do not have the necessary > > ACPI hooks to enable ASPM. However if the BIOS has requested ASPM > > enablement, we should try to honor that request regardless. This patch > > adds the ASPM support to VMD child devices if requested by the FADT > > table. > > ASPM is enabled by software but the actual link state transitions > between L0, L0s, L1, etc are done autonomously by hardware. > > There should be no functional difference between ASPM being disabled > vs being enabled. The only difference should be power consumption and > some latency. It seems there was some confusion on my end about the role of BIOS in setting ASPM defaults. > > There are no ACPI hooks required to enable ASPM. The > ACPI_FADT_NO_ASPM bit tells the OS that "it must not enable OSPM ASPM > control" (ACPI v6.3, sec 5.2.9.3). AFAIK there is nothing in ACPI > to request that the OS *should* enable ASPM. > I see how it's used and not needed. Thanks! > > Signed-off-by: Jon Derrick <jonathan.derrick@intel.com> > > Signed-off-by: You-Sheng Yang <vicamo.yang@canonical.com> > > --- > > > > Hi, > > > > My knowledge on these kinds of power modes is limited, and we are having > > trouble bringing the Root Port child device out of L1 with this patch. > > Presumably this patch helps something. Do you mean you're having > trouble *without* this patch? > > > Can you help me understand the correct flow for bringing the Root Port > > device out of L1 with kernel flow, and what I might be missing here? > > I don't understand the issue yet. L1 is a state of the link, not of > an individual device. The devices on both ends of the link, e.g., a > Root Port and an Endpoint or Switch Upstream Port below it, negotiate > to determine the link state. This all happens in hardware without > software involvement. > > The only software influence is to enable hardware to select certain > link states. The confusion seemed to be about enabling the link states, which seemed to have been cleared up as a BIOS responsibility for default ASPM policy. > > > drivers/pci/controller/vmd.c | 9 ++++++++- > > drivers/pci/pcie/aspm.c | 19 ++----------------- > > include/linux/pci.h | 17 +++++++++++++++++ > > 3 files changed, 27 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c > > index 76d8acbee7d5..f1b058efb642 100644 > > --- a/drivers/pci/controller/vmd.c > > +++ b/drivers/pci/controller/vmd.c > > @@ -14,6 +14,7 @@ > > #include <linux/srcu.h> > > #include <linux/rculist.h> > > #include <linux/rcupdate.h> > > +#include <linux/acpi.h> > > > > #include <asm/irqdomain.h> > > #include <asm/device.h> > > @@ -601,8 +602,14 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features) > > * and will fail pcie_bus_configure_settings() early. It can instead be > > * run on each of the real root ports. > > */ > > - list_for_each_entry(child, &vmd->bus->children, node) > > + list_for_each_entry(child, &vmd->bus->children, node) { > > +#if IS_ENABLED(CONFIG_PCIEASPM) > > + if (!(acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_ASPM)) > > + pcie_config_aspm_link(child->self->link_state, > > + ASPM_STATE_ALL); > > +#endif > > pcie_aspm_init_link_state() is called from pci_scan_slot() and should > be doing ASPM configuration without help from VMD. If that's not > happening, I would instrument pcie_aspm_init_link_state() to make sure > it's being called and to figure out if there's something there that > prevents ASPM config for children of the VMD device. > > > pcie_bus_configure_settings(child); > > + } > > > > pci_bus_add_devices(vmd->bus); > > > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c > > index 253c30cc1967..04cdb0b5a672 100644 > > --- a/drivers/pci/pcie/aspm.c > > +++ b/drivers/pci/pcie/aspm.c > > @@ -25,22 +25,6 @@ > > #endif > > #define MODULE_PARAM_PREFIX "pcie_aspm." > > > > -/* Note: those are not register definitions */ > > -#define ASPM_STATE_L0S_UP (1) /* Upstream direction L0s state */ > > -#define ASPM_STATE_L0S_DW (2) /* Downstream direction L0s state */ > > -#define ASPM_STATE_L1 (4) /* L1 state */ > > -#define ASPM_STATE_L1_1 (8) /* ASPM L1.1 state */ > > -#define ASPM_STATE_L1_2 (0x10) /* ASPM L1.2 state */ > > -#define ASPM_STATE_L1_1_PCIPM (0x20) /* PCI PM L1.1 state */ > > -#define ASPM_STATE_L1_2_PCIPM (0x40) /* PCI PM L1.2 state */ > > -#define ASPM_STATE_L1_SS_PCIPM (ASPM_STATE_L1_1_PCIPM | ASPM_STATE_L1_2_PCIPM) > > -#define ASPM_STATE_L1_2_MASK (ASPM_STATE_L1_2 | ASPM_STATE_L1_2_PCIPM) > > -#define ASPM_STATE_L1SS (ASPM_STATE_L1_1 | ASPM_STATE_L1_1_PCIPM |\ > > - ASPM_STATE_L1_2_MASK) > > -#define ASPM_STATE_L0S (ASPM_STATE_L0S_UP | ASPM_STATE_L0S_DW) > > -#define ASPM_STATE_ALL (ASPM_STATE_L0S | ASPM_STATE_L1 | \ > > - ASPM_STATE_L1SS) > > - > > struct aspm_latency { > > u32 l0s; /* L0s latency (nsec) */ > > u32 l1; /* L1 latency (nsec) */ > > @@ -748,7 +732,7 @@ static void pcie_config_aspm_dev(struct pci_dev *pdev, u32 val) > > PCI_EXP_LNKCTL_ASPMC, val); > > } > > > > -static void pcie_config_aspm_link(struct pcie_link_state *link, u32 state) > > +void pcie_config_aspm_link(struct pcie_link_state *link, u32 state) > > { > > u32 upstream = 0, dwstream = 0; > > struct pci_dev *child = link->downstream, *parent = link->pdev; > > @@ -798,6 +782,7 @@ static void pcie_config_aspm_link(struct pcie_link_state *link, u32 state) > > > > link->aspm_enabled = state; > > } > > +EXPORT_SYMBOL_GPL(pcie_config_aspm_link); > > > > static void pcie_config_aspm_path(struct pcie_link_state *link) > > { > > diff --git a/include/linux/pci.h b/include/linux/pci.h > > index 7a40cd5caed0..1c41781b160a 100644 > > --- a/include/linux/pci.h > > +++ b/include/linux/pci.h > > @@ -377,6 +377,22 @@ struct pci_dev { > > unsigned int d3cold_delay; /* D3cold->D0 transition time in ms */ > > > > #ifdef CONFIG_PCIEASPM > > +/* Note: those are not register definitions */ > > +#define ASPM_STATE_L0S_UP (1) /* Upstream direction L0s state */ > > +#define ASPM_STATE_L0S_DW (2) /* Downstream direction L0s state */ > > +#define ASPM_STATE_L1 (4) /* L1 state */ > > +#define ASPM_STATE_L1_1 (8) /* ASPM L1.1 state */ > > +#define ASPM_STATE_L1_2 (0x10) /* ASPM L1.2 state */ > > +#define ASPM_STATE_L1_1_PCIPM (0x20) /* PCI PM L1.1 state */ > > +#define ASPM_STATE_L1_2_PCIPM (0x40) /* PCI PM L1.2 state */ > > +#define ASPM_STATE_L1_SS_PCIPM (ASPM_STATE_L1_1_PCIPM | ASPM_STATE_L1_2_PCIPM) > > +#define ASPM_STATE_L1_2_MASK (ASPM_STATE_L1_2 | ASPM_STATE_L1_2_PCIPM) > > +#define ASPM_STATE_L1SS (ASPM_STATE_L1_1 | ASPM_STATE_L1_1_PCIPM |\ > > + ASPM_STATE_L1_2_MASK) > > +#define ASPM_STATE_L0S (ASPM_STATE_L0S_UP | ASPM_STATE_L0S_DW) > > +#define ASPM_STATE_ALL (ASPM_STATE_L0S | ASPM_STATE_L1 | \ > > + ASPM_STATE_L1SS) > > + > > struct pcie_link_state *link_state; /* ASPM link state */ > > unsigned int ltr_path:1; /* Latency Tolerance Reporting > > supported from root to here */ > > @@ -1577,6 +1593,7 @@ extern bool pcie_ports_native; > > #define PCIE_LINK_STATE_L1_2_PCIPM BIT(6) > > > > #ifdef CONFIG_PCIEASPM > > +void pcie_config_aspm_link(struct pcie_link_state *link, u32 state); > > int pci_disable_link_state(struct pci_dev *pdev, int state); > > int pci_disable_link_state_locked(struct pci_dev *pdev, int state); > > void pcie_no_aspm(void); > > -- > > 2.18.1 > >
diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c index 76d8acbee7d5..f1b058efb642 100644 --- a/drivers/pci/controller/vmd.c +++ b/drivers/pci/controller/vmd.c @@ -14,6 +14,7 @@ #include <linux/srcu.h> #include <linux/rculist.h> #include <linux/rcupdate.h> +#include <linux/acpi.h> #include <asm/irqdomain.h> #include <asm/device.h> @@ -601,8 +602,14 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features) * and will fail pcie_bus_configure_settings() early. It can instead be * run on each of the real root ports. */ - list_for_each_entry(child, &vmd->bus->children, node) + list_for_each_entry(child, &vmd->bus->children, node) { +#if IS_ENABLED(CONFIG_PCIEASPM) + if (!(acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_ASPM)) + pcie_config_aspm_link(child->self->link_state, + ASPM_STATE_ALL); +#endif pcie_bus_configure_settings(child); + } pci_bus_add_devices(vmd->bus); diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c index 253c30cc1967..04cdb0b5a672 100644 --- a/drivers/pci/pcie/aspm.c +++ b/drivers/pci/pcie/aspm.c @@ -25,22 +25,6 @@ #endif #define MODULE_PARAM_PREFIX "pcie_aspm." -/* Note: those are not register definitions */ -#define ASPM_STATE_L0S_UP (1) /* Upstream direction L0s state */ -#define ASPM_STATE_L0S_DW (2) /* Downstream direction L0s state */ -#define ASPM_STATE_L1 (4) /* L1 state */ -#define ASPM_STATE_L1_1 (8) /* ASPM L1.1 state */ -#define ASPM_STATE_L1_2 (0x10) /* ASPM L1.2 state */ -#define ASPM_STATE_L1_1_PCIPM (0x20) /* PCI PM L1.1 state */ -#define ASPM_STATE_L1_2_PCIPM (0x40) /* PCI PM L1.2 state */ -#define ASPM_STATE_L1_SS_PCIPM (ASPM_STATE_L1_1_PCIPM | ASPM_STATE_L1_2_PCIPM) -#define ASPM_STATE_L1_2_MASK (ASPM_STATE_L1_2 | ASPM_STATE_L1_2_PCIPM) -#define ASPM_STATE_L1SS (ASPM_STATE_L1_1 | ASPM_STATE_L1_1_PCIPM |\ - ASPM_STATE_L1_2_MASK) -#define ASPM_STATE_L0S (ASPM_STATE_L0S_UP | ASPM_STATE_L0S_DW) -#define ASPM_STATE_ALL (ASPM_STATE_L0S | ASPM_STATE_L1 | \ - ASPM_STATE_L1SS) - struct aspm_latency { u32 l0s; /* L0s latency (nsec) */ u32 l1; /* L1 latency (nsec) */ @@ -748,7 +732,7 @@ static void pcie_config_aspm_dev(struct pci_dev *pdev, u32 val) PCI_EXP_LNKCTL_ASPMC, val); } -static void pcie_config_aspm_link(struct pcie_link_state *link, u32 state) +void pcie_config_aspm_link(struct pcie_link_state *link, u32 state) { u32 upstream = 0, dwstream = 0; struct pci_dev *child = link->downstream, *parent = link->pdev; @@ -798,6 +782,7 @@ static void pcie_config_aspm_link(struct pcie_link_state *link, u32 state) link->aspm_enabled = state; } +EXPORT_SYMBOL_GPL(pcie_config_aspm_link); static void pcie_config_aspm_path(struct pcie_link_state *link) { diff --git a/include/linux/pci.h b/include/linux/pci.h index 7a40cd5caed0..1c41781b160a 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -377,6 +377,22 @@ struct pci_dev { unsigned int d3cold_delay; /* D3cold->D0 transition time in ms */ #ifdef CONFIG_PCIEASPM +/* Note: those are not register definitions */ +#define ASPM_STATE_L0S_UP (1) /* Upstream direction L0s state */ +#define ASPM_STATE_L0S_DW (2) /* Downstream direction L0s state */ +#define ASPM_STATE_L1 (4) /* L1 state */ +#define ASPM_STATE_L1_1 (8) /* ASPM L1.1 state */ +#define ASPM_STATE_L1_2 (0x10) /* ASPM L1.2 state */ +#define ASPM_STATE_L1_1_PCIPM (0x20) /* PCI PM L1.1 state */ +#define ASPM_STATE_L1_2_PCIPM (0x40) /* PCI PM L1.2 state */ +#define ASPM_STATE_L1_SS_PCIPM (ASPM_STATE_L1_1_PCIPM | ASPM_STATE_L1_2_PCIPM) +#define ASPM_STATE_L1_2_MASK (ASPM_STATE_L1_2 | ASPM_STATE_L1_2_PCIPM) +#define ASPM_STATE_L1SS (ASPM_STATE_L1_1 | ASPM_STATE_L1_1_PCIPM |\ + ASPM_STATE_L1_2_MASK) +#define ASPM_STATE_L0S (ASPM_STATE_L0S_UP | ASPM_STATE_L0S_DW) +#define ASPM_STATE_ALL (ASPM_STATE_L0S | ASPM_STATE_L1 | \ + ASPM_STATE_L1SS) + struct pcie_link_state *link_state; /* ASPM link state */ unsigned int ltr_path:1; /* Latency Tolerance Reporting supported from root to here */ @@ -1577,6 +1593,7 @@ extern bool pcie_ports_native; #define PCIE_LINK_STATE_L1_2_PCIPM BIT(6) #ifdef CONFIG_PCIEASPM +void pcie_config_aspm_link(struct pcie_link_state *link, u32 state); int pci_disable_link_state(struct pci_dev *pdev, int state); int pci_disable_link_state_locked(struct pci_dev *pdev, int state); void pcie_no_aspm(void);