Received-SPF: pass (google.com: domain of linux-kernel+bounces-54951-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
From: =?UTF-8?q?Ilpo=20J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>
Date: Tue, 6 Feb 2024 14:29:12 +0200 (EET)
To: Jian-Hong Pan <jhp@endlessos.org>
cc: Bjorn Helgaas <helgaas@kernel.org>, Johan Hovold <johan@kernel.org>, 
    Mika Westerberg <mika.westerberg@linux.intel.com>, 
    David Box <david.e.box@linux.intel.com>, 
    Damien Le Moal <dlemoal@kernel.org>, Niklas Cassel <cassel@kernel.org>, 
    Nirmal Patel <nirmal.patel@linux.intel.com>, 
    Jonathan Derrick <jonathan.derrick@linux.dev>, linux-pci@vger.kernel.org, 
    LKML <linux-kernel@vger.kernel.org>, linux@endlessos.org
Subject: Re: [PATCH v2] PCI: vmd: Enable PCI PM's L1 substates of remapped
 PCIe Root Port and NVMe
In-Reply-To: <20240202071110.8515-3-jhp@endlessos.org>
Message-ID: <72b9ef46-851b-3555-0a43-8da4d4ab0fad@linux.intel.com>
References: <20240202071110.8515-3-jhp@endlessos.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII

On Fri, 2 Feb 2024, Jian-Hong Pan wrote:

> The remapped PCIe Root Port and NVMe have PCI PM L1 substates
> capability, but they are disabled originally:
> 
> Here is an example on ASUS B1400CEAE:
> 
> Capabilities: [900 v1] L1 PM Substates
>         L1SubCap: PCI-PM_L1.2+ PCI-PM_L1.1- ASPM_L1.2+ ASPM_L1.1- L1_PM_Substates+
>                   PortCommonModeRestoreTime=32us PortTPowerOnTime=10us
>         L1SubCtl1: PCI-PM_L1.2- PCI-PM_L1.1- ASPM_L1.2+ ASPM_L1.1-
>                    T_CommonMode=0us LTR1.2_Threshold=0ns
>         L1SubCtl2: T_PwrOn=10us
> 
> Power on all of the VMD remapped PCI devices and quirk max snoop LTR
> before enable PCI-PM L1 PM Substates by following "Section 5.5.4 of PCIe
> Base Spec Revision 6.0". Then, PCI PM's L1 substates control are
> initialized & enabled accordingly. Also, update the comments of
> pci_enable_link_state() and pci_enable_link_state_locked() for
> kernel-doc, too.
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218394
> Signed-off-by: Jian-Hong Pan <jhp@endlessos.org>
> ---
> v2:
>   - Power on the VMD remapped devices with pci_set_power_state_locked()
>   - Prepare the PCIe LTR parameters before enable L1 Substates
>   - Add note into the comments of both pci_enable_link_state() and
>     pci_enable_link_state_locked() for kernel-doc.
>   - The original patch set can be split as individual patches.
> 
>  drivers/pci/controller/vmd.c | 15 ++++++++++-----
>  drivers/pci/pcie/aspm.c      | 10 ++++++++++
>  2 files changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index 87b7856f375a..66e47a0dbf1a 100644
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c
> @@ -751,11 +751,9 @@ static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata)
>  	if (!(features & VMD_FEAT_BIOS_PM_QUIRK))
>  		return 0;
>  
> -	pci_enable_link_state_locked(pdev, PCIE_LINK_STATE_ALL);
> -
>  	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_LTR);
>  	if (!pos)
> -		return 0;
> +		goto out_enable_link_state;
>  
>  	/*
>  	 * Skip if the max snoop LTR is non-zero, indicating BIOS has set it
> @@ -763,7 +761,7 @@ static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata)
>  	 */
>  	pci_read_config_dword(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, &ltr_reg);
>  	if (!!(ltr_reg & (PCI_LTR_VALUE_MASK | PCI_LTR_SCALE_MASK)))
> -		return 0;
> +		goto out_enable_link_state;
>  
>  	/*
>  	 * Set the default values to the maximum required by the platform to
> @@ -775,6 +773,14 @@ static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata)
>  	pci_write_config_dword(pdev, pos + PCI_LTR_MAX_SNOOP_LAT, ltr_reg);
>  	pci_info(pdev, "VMD: Default LTR value set by driver\n");
>  
> +out_enable_link_state:
> +	/*
> +	 * Make PCI devices at D0 when enable PCI-PM L1 PM Substates from
> +	 * Section 5.5.4 of PCIe Base Spec Revision 6.0

I don't understand what are you trying to say here? Are there some typos 
or grammar errors or something entire missing from the comment?

> +	 */
> +	pci_set_power_state_locked(pdev, PCI_D0);
> +	pci_enable_link_state_locked(pdev, PCIE_LINK_STATE_ALL);
> +
>  	return 0;
>  }
>  
> @@ -926,7 +932,6 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
>  				   dev_get_msi_domain(&vmd->dev->dev));
>  
>  	vmd_acpi_begin();
> -
>  	pci_scan_child_bus(vmd->bus);
>  	vmd_domain_reset(vmd);

Spurious newline change.


-- 
 i.