Re: [RFC] PCI: vmd: Enable ASPM if BIOS requests it

["Derrick, Jonathan" <jonathan.derrick@intel.com>]

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
> >