Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <1a163e75-f620-dfe7-01b1-5356fe767b2d@linux.dev>
Date:   Fri, 18 Feb 2022 14:30:48 -0700
MIME-Version: 1.0
Subject: Re: [PATCH V5 3/3] PCI: vmd: Configure PCIe ASPM and LTR
Content-Language: en-US
To:     "David E. Box" <david.e.box@linux.intel.com>,
        nirmal.patel@linux.intel.com, lorenzo.pieralisi@arm.com,
        hch@infradead.org, kw@linux.com, robh@kernel.org,
        bhelgaas@google.com, michael.a.bottini@linux.intel.com,
        rafael@kernel.org, me@adhityamohan.in
Cc:     linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20220218045056.333799-1-david.e.box@linux.intel.com>
 <20220218045056.333799-4-david.e.box@linux.intel.com>
From:   Jonathan Derrick <jonathan.derrick@linux.dev>
In-Reply-To: <20220218045056.333799-4-david.e.box@linux.intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 2/17/2022 9:50 PM, David E. Box wrote:
> Currently, PCIe ports reserved for VMD use are not visible to BIOS and
> therefore not configured to enable PCIE ASPM. Additionally, PCIE LTR
> values may be left unset since BIOS will set a default maximum LTR value
> on endpoints to ensure that misconfigured devices don't block SoC power
> management. Lack of this programming results in high power consumption
> on laptops as reported in bugzilla [1].  For currently affected
> products, use pci_enable_default_link_state to set the allowed link
> states for devices on the root ports. Also set the LTR value to the
> maximum value needed for the SoC. Per the VMD hardware team future
> products using VMD will enable BIOS configuration of these capabilities.
Will the refreshes of the affected CPU be supported in BIOS or this workaround?

> This solution is a workaround for current products that mainly targets
> laptops. Support is not provided if a switch is used nor for hotplug.
> 
> [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>
> ---
> V5
>   - Provide the LTR value as driver data.
>   - Use DWORD for the config space write to avoid PCI WORD access bug.
>   - Set ASPM links firsts, enabling all link states, before setting a
>     default LTR if the capability is present
>   - Add kernel message that VMD is setting the device LTR.
> V4
>   - Refactor vmd_enable_apsm() to exit early, making the lines shorter
>     and more readable. Suggested by Christoph.
> V3
>   - No changes
> V2
>   - Use return status to print pci_info message if ASPM cannot be enabled.
>   - Add missing static declaration, caught by lkp@intel.com
> 
>   drivers/pci/controller/vmd.c | 48 +++++++++++++++++++++++++++++++++++-
>   1 file changed, 47 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index a582c351b461..eac379c80cd7 100644
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c
> @@ -67,10 +67,19 @@ enum vmd_features {
>   	 * interrupt handling.
>   	 */
>   	VMD_FEAT_CAN_BYPASS_MSI_REMAP		= (1 << 4),
> +
> +	/*
> +	 * Enable ASPM on the PCIE root ports 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.
> +	 */
> +	VMD_FEAT_BIOS_PM_QUIRK		= (1 << 5),
>   };
>   
>   struct vmd_device_data {
>   	enum vmd_features features;
> +	u16 ltr;
>   };
>   
>   static DEFINE_IDA(vmd_instance_ida);
> @@ -714,6 +723,38 @@ static void vmd_copy_host_bridge_flags(struct pci_host_bridge *root_bridge,
>   	vmd_bridge->native_dpc = root_bridge->native_dpc;
>   }
>   
> +/*
> + * Enable ASPM and LTR settings on devices that aren't configured by BIOS.
> + */
> +static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata)
> +{
> +	struct vmd_device_data *info = userdata;
> +	u32 ltr_reg;
> +	int pos;
> +
> +	if (!(info->features & VMD_FEAT_BIOS_PM_QUIRK))
> +		return 0;
> +
> +	pci_enable_default_link_state(pdev, PCIE_LINK_STATE_ALL);
> +
> +	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_LTR);
> +	if (!pos)
> +		return 0;
> +
> +	/*
> +	 * If the LTR capability is present, set the default values to the
> +	 * maximum required by the platform to allow the deepest power
> +	 * management savings. Write this as a single DWORD where the lower word
> +	 * is the max snoop latency and the upper word is the max non-snoop
> +	 * latency.
> +	 */
> +	pci_info(pdev, "VMD: Setting a default LTR\n");
> +	ltr_reg = (info->ltr << 16) | info->ltr;
> +	pci_write_config_dword(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, ltr_reg);
> +
> +	return 0;
> +}
> +
>   static int vmd_enable_domain(struct vmd_dev *vmd, struct vmd_device_data *info)
>   {
>   	struct pci_sysdata *sd = &vmd->sysdata;
> @@ -867,6 +908,8 @@ static int vmd_enable_domain(struct vmd_dev *vmd, struct vmd_device_data *info)
>   		pci_reset_bus(child->self);
>   	pci_assign_unassigned_bus_resources(vmd->bus);
>   
> +	pci_walk_bus(vmd->bus, vmd_pm_enable_quirk, info);
> +
>   	/*
>   	 * 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
> @@ -1012,7 +1055,10 @@ static const struct vmd_device_data vmd_28c0_data = {
>   static const struct vmd_device_data vmd_467f_data = {
>   	.features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
>   		    VMD_FEAT_HAS_BUS_RESTRICTIONS |
> -		    VMD_FEAT_OFFSET_FIRST_VECTOR,
> +		    VMD_FEAT_OFFSET_FIRST_VECTOR |
> +		    VMD_FEAT_BIOS_PM_QUIRK,
> +	/* 3145728 ns (LatencyScale of 1048576 ns with a LatencyValue of 3) */
> +	.ltr = 0x1003,
>   };
>   
>   static const struct pci_device_id vmd_ids[] = {